|
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