97jaz / gregor

Date and time library for Racket
45 stars 10 forks source link

rename `build-comparison` functions #31

Closed bennn closed 5 years ago

bennn commented 5 years ago

Use the first argument to rename the =? <? .... functions.

EDIT: the original example in this pull request was bad

This helps improve error messages that use object-name, for example arity errors:

(require gregor)
(date<? 0)

Also (displayln date<?)

97jaz commented 5 years ago

I think this is fine, but do you think it would be worthwhile to do more work at expansion time? I recall a couple of cases in the not-so-distant past when people (including myself) have brought up rename-procedure on the mailing list, and the response has generally been to avoid it if possible, because of the runtime overhead. So something like:

#lang racket/base

(require data/order)

(provide define-comparison)

(define-syntax-rule
  (define-comparison pred? ->num (=? <? <=? >? >=? comparator order-name))
  (begin
    (define (comparator x y)
      (define diff (- (->num x) (->num y)))

      (cond [(negative? diff) '<]
            [(zero? diff)     '=]
            [else             '>]))

    (define order-name
      (order (quote order-name) pred? comparator))

    (define (=? x y) (eq? '= (comparator x y)))
    (define (<? x y) (eq? '< (comparator x y)))

    (define (<=? x y)
      (case (comparator x y)
        [(< =) #t]
        [else  #f]))

    (define (>=? x y)
      (case (comparator x y)
        [(> =) #t]
        [else #f]))))

And then replace the match-define with a call to this, passing in the names?

Or is this just not worthwhile?

bennn commented 5 years ago

Sure, let's use a macro!

I went with procedure-rename at first because it needed a smaller diff and used a runtime value to make the names. I wasn't thinking about performance. Avoiding it sounds like good advice.

The current PR here builds names during expansion.

EDIT: I've been testing using raco test -c gregor. Is that enough?

bennn commented 5 years ago

squashed

97jaz commented 5 years ago

Thanks!