From e34e001167077490a27fd9a1cd1ed5209c24d65c Mon Sep 17 00:00:00 2001
From: Eli Barzilay <eli@racket-lang.org>
Date: Thu, 12 Nov 2009 00:18:08 +0000
Subject: [PATCH] comments

svn: r16704
---
 collects/unstable/bytes.ss        | 12 ++++++++++++
 collects/unstable/contract.ss     |  4 ++++
 collects/unstable/exn.ss          |  1 +
 collects/unstable/list.ss         | 11 +++++++++++
 collects/unstable/mutated-vars.ss | 11 ++++++++++-
 collects/unstable/net/url.ss      | 13 +++++++++++++
 collects/unstable/path.ss         | 25 +++++++++++++++++++++++++
 collects/unstable/string.ss       |  9 +++++++++
 collects/unstable/struct.ss       | 10 ++++++++++
 collects/unstable/syntax.ss       | 16 +++++++++++++++-
 10 files changed, 110 insertions(+), 2 deletions(-)

diff --git a/collects/unstable/bytes.ss b/collects/unstable/bytes.ss
index 207e4b6469..d6da04b714 100644
--- a/collects/unstable/bytes.ss
+++ b/collects/unstable/bytes.ss
@@ -9,11 +9,23 @@
 (define (bytes-ci=? b0 b1)
   (string-ci=? (bytes->string/utf-8 b0)
                (bytes->string/utf-8 b1)))
+;; Eli: If this ever gets in, it should say that the memory requirements
+;;   are 4 times the input size, especially since bytes are often used to save
+;;   space.  Also, fails on (bytes-ci=? #"\277" #"\277"), and a trivial fix
+;;   would still fail on (bytes-ci=? #"\276\277" #"\277\276")
 
 (define (read/bytes bs)
   (read (open-input-bytes bs)))
+;; Eli: This is a really bad name for something that is often called
+;;   `read-from-string', or `read-from-bytes' in this case.  I first read it as
+;;   "read with bytes".  Regardless, I see little point in composing two
+;;   functions where the two names are clear enough -- you might consider
+;;   looking at the version in CL.
 
 (define (write/bytes v)
   (define by (open-output-bytes))
   (write v by)
   (get-output-bytes by))
+;; Eli: Same bad name as above.  Also, is there any point in this given
+;;   (format "~s" v), and the fact that using the resulting string for printout
+;;   will get the same result.
diff --git a/collects/unstable/contract.ss b/collects/unstable/contract.ss
index e01359b6b8..a7b22b9c50 100644
--- a/collects/unstable/contract.ss
+++ b/collects/unstable/contract.ss
@@ -2,12 +2,16 @@
 
 (define path-element?
   (or/c path-string? (symbols 'up 'same)))
+;; Eli: We already have a notion of "path element" which is different
+;;   from this (see `string->path-element') .
 
 (define port-number? (between/c 1 65535))
 
 (define non-empty-string/c
   (and/c string?
          (lambda (s) (not (zero? (string-length s))))))
+;; Eli: If this gets in, there should also be versions for bytes, lists, and
+;;   vectors.
 
 (provide/contract
  [non-empty-string/c contract?]
diff --git a/collects/unstable/exn.ss b/collects/unstable/exn.ss
index 84c752d094..e20207676f 100644
--- a/collects/unstable/exn.ss
+++ b/collects/unstable/exn.ss
@@ -14,6 +14,7 @@
         ((error-display-handler) (exn-message exn) exn)
         (get-output-string (current-error-port)))
       (format "~s\n" exn)))
+;; Eli: (or/c exn any)??
 
 (provide/contract
  [network-error ((symbol? string?) (listof any/c) . ->* . (void))]
diff --git a/collects/unstable/list.ss b/collects/unstable/list.ss
index cc984d8e69..1ecf6aab41 100644
--- a/collects/unstable/list.ss
+++ b/collects/unstable/list.ss
@@ -14,6 +14,17 @@
         (if (equal? l0 r0)
             (list-prefix? ls rs)
             #f)])]))
+;; Eli: Is this some `match' obsession syndrom?  The simple definition:
+;;   (define (list-prefix? ls rs)
+;;     (or (null? ls) (and (pair? rs) (equal? (car ls) (car rs))
+;;                         (list-prefix? (cdr ls) (cdr rs)))))
+;;   is shorter, and faster.  As for making this a library function: how
+;;   about a version that removes the equal prefix from two lists and
+;;   returns the tails -- this way you can tell if they're equal, or one
+;;   is a prefix of the other, or if there was any equal prefix at all.
+;;   (Which can be useful for things like making a path relative to
+;;   another path.)  A nice generalization is to make it get two or more
+;;   lists, and return a matching number of values.
 
 (provide/contract
  [list-prefix? (list? list? . -> . boolean?)])
diff --git a/collects/unstable/mutated-vars.ss b/collects/unstable/mutated-vars.ss
index 1e141c546d..efa9b39e49 100644
--- a/collects/unstable/mutated-vars.ss
+++ b/collects/unstable/mutated-vars.ss
@@ -42,5 +42,14 @@
 ;; is-var-mutated? : identifier -> boolean
 (define (is-var-mutated? id) (module-identifier-mapping-get table id (lambda _ #f)))
 
-(provide find-mutated-vars is-var-mutated?)
+;; Eli:
+;; - The `for-template' doesn't look like it's needed.
+;; - This is the *worst* looking interface I've seen in a while.  Seems very
+;;   specific to some unclear optimization needs.  (Either that, or translated
+;;   from C.)
+;; - Besides weird, identifiers maps are (IIRC) not weak, which makes this even
+;;   less general.
+;; - What's with the typed-scheme literals?  If they were needed, then
+;;   typed-scheme is probably broken now.
 
+(provide find-mutated-vars is-var-mutated?)
diff --git a/collects/unstable/net/url.ss b/collects/unstable/net/url.ss
index e22cbbd3c3..11e55a0138 100644
--- a/collects/unstable/net/url.ss
+++ b/collects/unstable/net/url.ss
@@ -22,6 +22,15 @@
      new-path
      empty
      (url-fragment in-url))))
+;; Eli: if it also removes the query, this it's a bad name, and it's
+;;   questionable whether it is general enough.  Why not make it into a
+;;   keyworded function that can change any part, which sounds like a much more
+;;   useful utility?  Some `foo' that would allow:
+;;     (define (url-replace-path proc in-url)
+;;       (foo in-url #:path (proc (url-path in-url)) #:query '()))
+;;   or even accept a changing function for all keywords:
+;;     (define (url-replace-path proc in-url)
+;;       (foo in-url #:path proc #:query '()))
 
 ;; ripped this off from url-unit.ss
 (define (url-path->string strs)
@@ -42,3 +51,7 @@
               [(up)   ".."]
               [else (error 'maybe-join-params
                            "bad value from path/param-path: ~e" s)])))))
+;; Eli: I don't know what this is supposed to be doing -- I don't see any
+;;   "maybe"ness), it throws away the `path/param-param's, and it accepts
+;;   strings too (which makes me wonder how is this related to the url
+;;   library).
diff --git a/collects/unstable/path.ss b/collects/unstable/path.ss
index 197c020851..965f6055b1 100644
--- a/collects/unstable/path.ss
+++ b/collects/unstable/path.ss
@@ -11,6 +11,8 @@
       [else
        (let-values ([(base name dir?) (split-path p)])
          (loop base (list* name r)))])))
+;; Eli: We already have `explode-path', this looks like it's doing the
+;;   same thing, except a little less useful.
 
 ; strip-prefix-ups : (listof path-element?) -> (listof path-element?)
 (define (strip-prefix-ups l)
@@ -23,6 +25,18 @@
                            (set-box! prefix? #f)))
                 #t))
           l))
+;; Eli: This is bad.  If I understand it correctly, this is what this
+;;   *should* have been:
+;;     (define (strip-prefix-ups l)
+;;       (if (and (pair? l) (eq? 'up (car l))) (strip-prefix-ups (cdr l)) l))
+;;   or even:
+;;     (define (strip-prefix-ups l)
+;;       (match l [(cons 'up l) (strip-prefix-ups l)] [_ l]))
+;;   except that the above version manages to combine ugly and
+;;   obfuscated code, redundant mutation, redundant code (why is it a
+;;   box? why is there a (begin #t ...)?), and being extra slow.  Oh,
+;;   and if this wasn't enough, there's exactly one place in the web
+;;   server that uses it.
 
 ; path-without-base : path? path? -> (listof path-element?)
 (define (path-without-base base path)
@@ -31,12 +45,19 @@
   (if (list-prefix? b p)
       (list-tail p (length b))
       (error 'path-without-base "~a is not a prefix of ~a" base path)))
+;; Eli: see my comment on `list-prefix?' -- it would make this trivial.
+;;   Also, if you want to look for a useful utility to add, search the code for
+;;   `relativize', which is a popular thing that gets written multiple times
+;;   and would be nice to have as a library.  (But there are some differences
+;;   between them, I think.)
 
 ;; build-path-unless-absolute : path-string? path-string? -> path?
 (define (build-path-unless-absolute base path)
   (if (absolute-path? path)
       (build-path path)
       (build-path base path)))
+;; Eli: This looks completely unnecessary.  I find the code much easier to
+;;   understand than the long name.
 
 (define (directory-part path)
   (let-values ([(base name must-be-dir) (split-path path)])
@@ -44,6 +65,10 @@
       [(eq? 'relative base) (current-directory)]
       [(not base) (error 'directory-part "~a is a top-level directory" path)]
       [(path? base) base])))
+;; Eli: There is now a `file-name-from-path', which suggests that the name for
+;;   this should be `directory-name-from-path', but perhaps a new name is
+;;   better for both.  Also, I find it questionable to return the current
+;;   directory in the first case.
 
 (provide/contract
  [explode-path* (path-string? . -> . (listof path-element?))]
diff --git a/collects/unstable/string.ss b/collects/unstable/string.ss
index 79f319d550..5bc4e3575b 100644
--- a/collects/unstable/string.ss
+++ b/collects/unstable/string.ss
@@ -3,10 +3,14 @@
 
 (define (read/string str)
   (read (open-input-string str)))
+;; Eli: Same comments as `read/bytes'.
+
 (define (write/string v)
   (define str (open-output-string))
   (write v str)
   (get-output-string str))
+;; Eli: Same comments as `write/string', and worse -- this is the same as
+;;   (format "~s" v)
 
 ; lowercase-symbol! : (or/c string bytes) -> symbol
 (define (lowercase-symbol! s)
@@ -15,6 +19,11 @@
     (if (bytes? s)
         (bytes->string/utf-8 s)
         s))))
+;; Eli: This doesn't make any sense at all.  Why is the `!' in the name?  Why
+;;   does it accept bytes?  Why does a function in a "string" library accept
+;;   bytes?  How can I guess that this creates a new symbol from that name?
+;;   (Which makes me think that this is (compose string->symbol string-downcase
+;;   symbol->string))
 
 (provide/contract
  [lowercase-symbol! ((or/c string? bytes?) . -> . symbol?)]
diff --git a/collects/unstable/struct.ss b/collects/unstable/struct.ss
index 33edfa4557..9d07b3f551 100644
--- a/collects/unstable/struct.ss
+++ b/collects/unstable/struct.ss
@@ -40,6 +40,11 @@
          (syntax-property #'(constructor expr ...)
                           'disappeared-use
                           #'S)))]))
+;; Eli: You give a good point for this, but I'd prefer if the optimizer would
+;;   detect these, so you'd get the same warnings for constructors too when you
+;;   use `-W warning'.  (And then, if you really want these things to be
+;;   errors, then perhaps something at the mzscheme level should make it throw
+;;   errors instead of warnings.)
 
 (define dummy-value (box 'dummy))
 
@@ -53,3 +58,8 @@
                   #f]
                  [else #t]))
          (cdr (vector->list vec)))))
+;; Eli: Why is there that `false-on-opaque?' business instead of having
+;;   an interface similar to `struct->vector'?  I'd prefer an optional
+;;   on-opaque value, and have it throw an error if it's opaque and no
+;;   value is given.  Also, `gensym' seems much better to me than a box
+;;   for a unique value.
diff --git a/collects/unstable/syntax.ss b/collects/unstable/syntax.ss
index b5599f651d..b46794d3b3 100644
--- a/collects/unstable/syntax.ss
+++ b/collects/unstable/syntax.ss
@@ -41,6 +41,9 @@
                 (apply make-prefab-struct key
                        (loop (struct->list x))))]
           [else x])))
+;; Eli: Is there any difference between this (with the default) and
+;;   `syntax->datum'?  If not, then maybe add the optional (or keyword) to
+;;   there instead?
 
 ;; Defining pattern variables
 
@@ -82,6 +85,7 @@
 (define-syntax-rule (with-temporaries (temp-name ...) . body)
   (with-syntax ([(temp-name ...) (generate-temporaries (quote-syntax (temp-name ...)))])
     . body))
+;; Eli: +1 to this, not sure about the next two
 
 ;; generate-temporary : any -> identifier
 (define (generate-temporary [stx 'g])
@@ -106,7 +110,16 @@
   (let* ([str (apply format fmt args)]
          [sym (string->symbol str)])
     (datum->syntax lctx sym src props cert)))
-
+;; Eli: This looks very *useful*, but I'd like to see it more convenient to
+;;   "preserve everything".  Maybe add a keyword argument that when #t makes
+;;   all the others use values lctx, and when syntax makes the others use that
+;;   syntax?  Also, I'd prefer it if each of these keywords would also accept a
+;;   syntax instead of a value, to copy the value from.
+;;   Finally, if you get to add this, then another useful utility in the same
+;;   spirit is one that concatenates symbols and/or strings and/or identifiers
+;;   into a new identifier.  I considered something like that, which expects a
+;;   single syntax among its inputs, and will use it for the context etc, or
+;;   throw an error if there's more or less than 1.
 
 ;; Error reporting
 
@@ -122,3 +135,4 @@
                         ctx
                         stx
                         extras)))
+;; Eli: The `report-error-as' thing seems arbitrary to me.