From 9227bfaf4c87997d289041f59868b2438a53571d Mon Sep 17 00:00:00 2001 From: Eli Barzilay Date: Fri, 27 Aug 2010 13:20:43 -0400 Subject: [PATCH] Comments on unsatbel code. --- collects/data/queue.rkt | 2 +- collects/unstable/dict.rkt | 58 ++++++++++++++++++++++++++++++++++++++ collects/unstable/hash.rkt | 10 +++++++ collects/unstable/list.rkt | 28 ++++++++++-------- collects/unstable/set.rkt | 8 ++++++ 5 files changed, 93 insertions(+), 13 deletions(-) diff --git a/collects/data/queue.rkt b/collects/data/queue.rkt index 8121ee84b1..6c733178d1 100644 --- a/collects/data/queue.rkt +++ b/collects/data/queue.rkt @@ -41,7 +41,7 @@ (define nonempty-queue/c (flat-named-contract "nonempty-queue" nonempty-queue?)) -;; ELI: Are these needed? (vs just providing `queue?', `make-queue' and +;; Eli: Are these needed? (vs just providing `queue?', `make-queue' and ;; `queue-empty?'.) (provide/contract [queue/c flat-contract?] diff --git a/collects/unstable/dict.rkt b/collects/unstable/dict.rkt index 0ad366de85..6d99b492e5 100644 --- a/collects/unstable/dict.rkt +++ b/collects/unstable/dict.rkt @@ -10,6 +10,9 @@ (define (dict-empty? dict) (= (dict-count dict) 0)) +;; Eli: This encourages ignoring the actual representation, and the fact +;; that `dict-count' can in some cases be an O(N) operation. (And to +;; make things worse, it's not even mentioned in the docs.) ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; ;; @@ -35,6 +38,17 @@ ([#t #t 'eq] (make-weak-hash)) ;; Impossible ([#f #t _] (error 'empty-set "cannot create an immutable weak hash")))) +;; Eli: What's the point in this? The whole dict thing is very similar +;; to an abstract class, and this code is essentially making a +;; constructor for the abstract class that decides to instantiate some +;; arbitrary subclass. Furthermore, since this arbitrary decision is +;; always going for a hash table, this whole function is nothing more +;; than a keyworded version of `make-hash-*'. (As a very obvious +;; example, if I have a mental model of alists, using this function +;; makes things much less efficient than just returning `null'.) This +;; is possibly something useful, but calling it `make-dict' is bogus. +;; Another evidence for this bogosity: the documentation for this +;; function says: "Constructs an empty >>hash table<<". (define (make-dict dict #:weak? [weak? #f] @@ -44,6 +58,11 @@ (if mutable? (begin (dict-union! MT dict) MT) (dict-union MT dict)))) +;; Eli: Similar bogosity to the above. When I see `make-dict', I don't +;; think about a function that "Converts a given dictionary to a hash +;; table". If it's useful, then it should have a more straightforward +;; name, like `dict->hash'. Also, reusing `dict-union' is cute, but +;; makes it slower than it could be. (define (custom-dict equiv? [hash1 (lambda (x) 0)] @@ -55,6 +74,20 @@ ([#t #f] (make-custom-hash equiv? hash1 hash2)) ([#t #t] (make-weak-custom-hash equiv? hash1 hash2)) ([#f #t] (error 'custom-set "cannot create an immutable weak hash")))) +;; Eli: Again, same bogosity comment applies here. Another point here: +;; using 0 for the default hashing functions sounds like a very bad idea +;; -- something that people will run into in the form of extremely bad +;; performance. In this case the docs do mention this -- but why not +;; use the default hash functions that racket already provides? Also, +;; the docs indicate that the degenerate hash function makes it +;; equivalent to a list-based dictionary, which is wrong: relying on +;; this seems bad (in case custom hashes are (or will be) more +;; sophisticated), and also it's equivalent to a list-based dictionary, +;; except with a costly constant factor for the hash machinery, and +;; without the advantages of an alist (order). In short, the docs +;; should really say "don't use this without hash functions" -- or +;; better, use the better hash functions as a default *or* don't make +;; them optional (at least the first one). ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; ;; @@ -62,8 +95,20 @@ ;; ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; +;; Eli: Ugh. So the above constructors are keyworded versions of hash +;; constructors in various forms, and here we take a *single* function +;; from the dict interface and split it into multiple functions? Is +;; there any point for this? If I were told just this high-level +;; description, I'd assume that an obvious motivation for doing this +;; would be performance, but in this case performance is lost. I also +;; lose the abolity to have a lazily computed default on the way, since +;; the default in `dict-ref/default' is a plain argument. The only new +;; thing here is the questionable `dict-ref/identity' (at least I have +;; never seen any code where something like that would be useful). + (define (dict-ref/check dict key) (dict-ref dict key)) +;; Eli: why the eta-expanded definition? (define (dict-ref/identity dict key) (dict-ref dict key (lambda () key))) @@ -73,6 +118,7 @@ (define (dict-ref/failure dict key failure) (dict-ref dict key (lambda () (failure)))) +;; Eli: Um, why (lambda () (failure)) and not just `failure'?? ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; ;; @@ -83,6 +129,12 @@ (define ((dict-duplicate-error name) key value1 value2) (error name "duplicate values for key ~e: ~e and ~e" key value1 value2)) +;; Eli: If this is useful, then at least make it worth using instead of +;; writing your own code. For example, inspect the arguments and choose +;; an efficient order for the loops, or use a temporary hash table for a +;; union of two alists, etc. Alternatively, just write a function for +;; merging two hash tables (and call it `hash-union', of course). + (define (dict-union #:combine [combine #f] #:combine/key [combine/key @@ -113,6 +165,12 @@ ;; ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; +;; Eli: Uh, what is this for? The documentation for this is unclear: it +;; has the technical details of what this is doing, but no explanation +;; about when this is useful. Specifically, it's unclear why I would +;; ever want a wrapped dictionary. (My best guess would be "something +;; that chaperons are a better solution for".) + (define (wrapped-dict-property #:unwrap unwrap #:wrap [wrap #f] diff --git a/collects/unstable/hash.rkt b/collects/unstable/hash.rkt index 938c087063..a757d131db 100644 --- a/collects/unstable/hash.rkt +++ b/collects/unstable/hash.rkt @@ -2,6 +2,8 @@ (require (for-syntax syntax/parse)) +;; Eli: See comments for `dict-ref/check' and relatives. + (define (hash-ref/check table key) (hash-ref table key)) @@ -14,6 +16,14 @@ (define (hash-ref/failure table key failure) (hash-ref table key (lambda () (failure)))) +;; Eli: See comment for `dict-union' and `dict-union!' -- these two do +;; make sense, but if they're in, then generalizing them to dictionaries +;; seems to make little sense. If they are useful, I'd much rather see +;; a more direct connection -- for example, make the dict functions +;; convert all dicts to hashes and then union them -- this will also +;; make the performance cost more obvious (and will actually be faster +;; in most cases of non-hash dictionaries). + (define ((hash-duplicate-error name) key value1 value2) (error name "duplicate values for key ~e: ~e and ~e" key value1 value2)) diff --git a/collects/unstable/list.rkt b/collects/unstable/list.rkt index 4bd6d12212..a14d7cdde7 100644 --- a/collects/unstable/list.rkt +++ b/collects/unstable/list.rkt @@ -11,18 +11,12 @@ (equal? (car ls) (car rs)) (list-prefix? (cdr ls) (cdr rs))))) -;; 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. -;; ryanc: changed to use Eli's version +;; Eli: 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. (define (internal-split-common-prefix as bs same? keep-prefix?) (let loop ([as as] [bs bs]) @@ -111,6 +105,16 @@ (car items) (loop (cdr items) (cons key-item sofar))))))) +;; Eli: Just to have a record of this: my complaint about having this +;; code separately from `remove-duplicates' still stands. Specifically, +;; that function decides when to use a hash table to make things faster, +;; and this code would benefit from the same. It would be much better +;; to extend that function so it can be used for both tasks rather than +;; a new piece of code that does it (only do it in a worse way, re +;; performance). Doing this can also benefit `remove-duplicates' -- for +;; example, make it accept a container so that users can choose how +;; when/if to use a hash table. + ;; sam added from carl (define-syntax (values->list stx) diff --git a/collects/unstable/set.rkt b/collects/unstable/set.rkt index bdc3547e67..24dff5411f 100644 --- a/collects/unstable/set.rkt +++ b/collects/unstable/set.rkt @@ -11,10 +11,18 @@ (define (set=? one two) (and (subset? one two) (subset? two one))) +;; Eli: Seeing the code in "racket/set.rkt", my guess is that this could +;; be much faster if it used two `set-subtract' instead. Also, using +;; `set-count' would make this *much* faster since the common case is +;; two sets with different number of elements. (And if the code moves +;; into "racket/set.rkt", then it could be even faster by using the +;; representation directly.) (define (proper-subset? one two) (and (subset? one two) (not (subset? two one)))) +;; Eli: Same comment here -- both re using `set-subtract', and using the +;; count first. (define (set-exclusive-or s0 . rest) (for/fold ([s s0]) ([s* (in-list rest)])