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)

SRFI 146 draft 12 comments (long) Sudarshan S Chawathe 14 Apr 2018 21:05 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