SRFI 146 draft 12 comments (long) Sudarshan S Chawathe (14 Apr 2018 21:06 UTC)
|
Re: SRFI 146 draft 12 comments (long)
John Cowan
(17 Apr 2018 04:13 UTC)
|
Re: SRFI 146 draft 12 comments (long)
Marc Nieper-Wißkirchen
(14 May 2018 15:38 UTC)
|
Re: SRFI 146 draft 12 comments (long)
Arthur A. Gleckler
(14 May 2018 19:25 UTC)
|
Re: SRFI 146 draft 12 comments (long)
Arthur A. Gleckler
(18 May 2018 00:35 UTC)
|
These notes are for draft #12 2018-04-11. Sorry for length of this message. All the comments are along the lines of minor questions or clarifications, and a a couple of small suggestions. ** Minor clarifications/suggestions 1. In the example illustrating incorrect use of mapping1 after it has been used as a parameter for mapping-set!, my interpretation is (based on "is an error" terminology earlier) that mapping1's value can be arbitrary at the end, including mappings other than those outlined in the example, and perhaps also non-mapping objects (although that may be weird). Is that correct? 2. In several places the freedom to mutate in the "!" procedures is noted by the phrase "... except that it is permitted to mutate and return the mapping argument rather than allocating a new mapping." My interpretation is that the "and" in that phrase is used in the sense of "and/or"; e.g., an implementation may mutate the argument but (for whatever reason) then decide to return a freshly allocated mapping anyway. I don't have a concrete scenario for such an implementation behavior, but the question is whether it would be considered valid. 3. The description of mapping-find says: "There are no guarantees how many times and with which keys and values predicate is invoked, but see below:" I don't understand to what the "but see below" part refers. The immediately following sentence is related, but seems to address an earlier point, not one about predicate invocations. Maybe something got deleted by accident? 4. I assume the comment about predicate invocations (above) also applies to mapping-count, mapping-any?, and mapping-every?. 5. (very minor) For mapping-split, I don't think I understand the motivation for returning five values (< <= = >= >) instead of just three (< = >). I see the connection of the 5 values with the earlier 5 procedures, but the argument in favor of only 3 is that those three will be disjoint (and if the <= and >= ones are needed, then mapping-union can be conveniently used). 6. Would it make sense to have a linear-update version of mapping-split (especially given the presence of mapping-catenate! later)? ** Very minor typos, suggestions, etc. 1. typo in 2nd para of Comparator restrictions section: recommanded -> recommended 2. In description of mapping-ref: "... then the value itself is return." -> "... then the value itself is returned." 3. The description of mapping-adjoin reads: "It is an error to add an element to mapping that does not return #t when passed to the type test procedure of the comparator." I think "element to mapping" should be replaced by "association to mapping whose key" or something to that effect. 4. The above comment also applies to the text following (mapping-set mapping arg ...). 5. In the description of mapping-replace, there is a "set" that should read "mapping". 6. In the description of hashmap-pop following that of mapping-pop: It may be clearer to replace "returns an arbitrary association" with "is similar but chooses an arbitrary association". 7. In the first sentence of the description of mapping-map: It may be clearer to move ", which returns two values, " to just after "Applies proc". 8. In the paragraph after the example of mapping-map: "duplicate elements" should read "duplicate associations" for consistency. Also, I think "in the sense of the /mapping/'s comparator's equality predicate" should read "in the sense of /comparator/'s equality predicate" (where I'm using /foo/ to denote italic font). That is, what matters is whether the keys are considered duplicates by the newly given comparator, not the comparator of the original mapping. 9. In the description of mapping-fold: "... nil is used as the second argument" should read "... nil is used as the third argument". 10. In the description of mapping-remove: "just the elements" should read "just the associations". 11. In the description of mapping-copy: "elements" should read "associations" for consistency. 12. The second paragraph of the Submappings section: I am confused by the uses of "entries" and "associations". My assumption was that the two terms are equivalent (although "associations" seems to be used much more often). But then the use of "entries" in the last sentence must refer to a different sense ((key value) components?) of the term. 13. Set theory operations section: My understanding is that the equality test used for comparing associations for these operations is different from the one used in the earlier group of operations (Submappings), in the sense that the Set theory operations ignore the values of (key, value) pairs when comparing associations. This seems obvious from the lack of an additional comparator argument (for values) for these operations, but may be worth noting nonetheless. 14. Also at above location: In the last sentence in the paragraph (about duplicate keys), it may be better to use "in the sense of the mappings' comparator" (more consistent with earlier use, although it will be the comparator's equality predicate that is used). 15. The above two comments also apply to the paragraph describing the linear-update versions of the set theory operations. It may be simpler to just omit the repeated details and say that those operations are the linear-update versions of the corresponding ones. 16. mapping-min-value (and mapping-max-value): I agree with the Note there, but the names of these procedures seem like they would invite bugs due to the obvious misunderstanding. Perhaps a name that is harder to misinterpret, even if more cumbersome, may be preferable: something like mapping-min-key-value or mapping-min-entry-value. (I realize these can also be misinterpreted, but hopefully the lack of parallelism with mapping-min-key makes things a bit better.) 17. Would it make sense to include mapping-min-entry and mapping-max-entry, each returning two values (key and value) with the obvious semantics? They seem natural given the other procedures (such as mapping-entries alongside mapping-keys and mapping-values). 18. A similar comment (returning both key and value) applies to mapping-key-predecessor and mapping-key-successor, although perhaps less so since in those cases there are no ...-value-... versions either. I don't have a concrete example at hand, but it seems that in some situations not returing the value with the key may result in an additional traversal in the underlying tree or similar data structure to get the value. (Granted that it will most likely be O(log n) and so not that big a deal.) 19. In the Note following mapping-range> "map" should read "mapping". 20. Implementation section: The summary outlines the setup prerequisites for the (srfi 146) implementation but not for the (srfi 146 hash) implementation. The (hamt) library also uses (srfi 16), (srfi 125), (srfi 143), and (srfi 151). There may be other dependencies too, I didn't have a chance to check properly yet. ** Minor notes on implementation (code). 1. I tested successfully (expected passes all pass) with Chibi and am trying to test also with Kawa (ongoing---chasing some dependencies). 2. The test suite does not contain a test for mapping-search (although I guess it is tested indirectly by its use in the implementation of other procedures), so I wrote a simple one that may be useful to include: (test-equal '("success updated" "failure ignored" ((0 . "zero") (1 . "one") (2 . "two [seen]") (3 . "three") (4 . "four") (5 . "five"))) (let ((m1 (mapping (make-default-comparator) 1 "one" 3 "three" 0 "zero" 4 "four" 2 "two" 5 "five"))) (define (f/ignore insert ignore) (ignore "failure ignored")) (define (s/update key val update remove) (update key (string-append val " [seen]") "success updated")) (let*-values (((m2 v2) (mapping-search m1 2 f/ignore s/update)) ((m3 v3) (mapping-search m2 42 f/ignore s/update))) (list v2 v3 (mapping->alist m3))))) Regards, -chaw