Re: [boost] [Formal review] Collection Traits library

From: Rob Stewart (stewart_at_[hidden])
Date: 2004-05-04 11:53:13

Do you think the library should be accepted as a Boost library?

Definitely.

________________
What is your evaluation of the design?

Excellent.

________________
What is your evaluation of the implementation?

Generally clean and straightforward.

I'd prefer "primary specialization" to "default" in the comments.

Lines longer than 80 columns in a number of files.
detail/common.hpp is particularly hard to view in 80 columns.

Inconsistent indentation among the files.

I question the value of separating iterator_of and
const_iterator_of in separate files given how similar they are.

Wrong comment on std::pair specialization of result_iterator_of.

Why isn't result_iterator_of implemented in terms of iterator_of
and const_iterator_of?  As it stands, it duplicates the code in
iterator.hpp and const_iterator.hpp.  Collocating their
implementation would reduce the chances of maintenance errors.

Extra indentation on non-MSVC version of the
std::istream_iterator's size function (size.hpp).

Couldn't the primary specialization of size_type be to use SFINAE
to check for a nested size_type type or else std::size_t?  You
wouldn't need any specializations then.

"Sise" [sic] is misspelled in sizer.hpp.  Shouldn't sizer.hpp be
in detail?

Indentation in the detail files could be lessened with "namespace
boost { namespace collection_traits_detail {".

Extra "size help" comment block at tail of
detail/implementation_help.hpp.

________________
What is your evaluation of the documentation?

The documentation is, overall, very good.  Some key points are
listed here.  Detailed comments and corrections appear at the end
of this message.

The Introduction is missing a motivation section.  It goes from a
short description to an example.  It should help the reader
understand why the library is valuable.

The Introduction is missing discussion of the use of namespace
scope functions to do what heretofore would be done via member
functions.  Instead, the example and the sentence immediately
following it imply this change of syntax.  Using the namespace
scope functions is central to the library, so it should be stated
early and clearly.

________________
What is your evaluation of the potential usefulness of the
library?

Highly useful.

________________
Did you try to use the library?

No.

________________
How much effort did you put into your evaluation?

More than an hour.

________________
Are you knowledgeable about the problem domain?

Yes.

________________________________________________________________
Documentation Comments

________________
Collection.htm
  All other HTML files use lowercase names and the .html suffix
  Collection
    Description
      bullet 1: Change "It" to "A Collection"
      bullet 1: Change "should cover" to "must exceed"
      bullet 2: "Semantics" is plural, but I'd suggest rewriting
        as, "There are no requirements for how or whether a
        Collection is copyable."
      para 2: Change "by-reference" to "by reference"
    Associated types
      row 1: Delete "Otherwise."  Unless I'm mistaken, the value
        type must be CopyConstructible regardless of whether the
        Collection is mutable.
      row 2: The iterator type must be *at least* an
        InputIterator, right?
      row 3: Change "not to modify" to "not modify" to make it
        read better
      row 4: Change "reference" to "non-const reference" for
        clarity
      row 6: Change "pointer" to "non-const pointer" for clarity
      rows 4-6: "Behaves like/as" is rather vague.  I'm not
        entirely certain what you mean to say so I can't offer
        suggestions at this time.
    Notation - this appears after the first use of "X" in the
      preceding table
    Expression semantics
      Omit the first column to leave more room for the others;
        the names are already given in the "Valid expressions"
        table.
      row 1-2: "Past-the-end" is not defined and could be
        interpreted to mean any distance past the end; this is a
        well understood notion, so there may be no reason to do
        more than you have.  I'm just trying to be thorough.
    Complexity guarantees
      empty() should be listed on its own line.
    Notes
      What do you mean by "equivalent to?"  Clearly, they needn't
        be the same type, but what operations must the reference
        type provide to be "equivalent to" the value type?

________________
index.html
  Introduction
    para 1: How about "ExtrinsicCollectionConcept" instead of
      "ExternalCollectionConcept?"
    para 2: Missing colon at EOL
    para 3: Change to read, "Here is a small example illustrating
      the use of Collection Traits (the full example can be found
      here):"
    example code: Inconsistent template parameters:
      "ExternalCollection" versus "EC"
    para 4: To the casual reader, there are no type generators in
      the example; everything looks like a "free-standing
      function."  Better might be to change to read, "By using
      the facilities of this library, my_generic_replace()
      automatically works for all Collections."
    para 4: Change the last sentence to read, "See The Forwarding
      Problem for information on why there are two versions of
      find() in the example."  That way, you don't confuse folks
      in the Introduction with "we cannot forward a non-const
      rvalue with reference arguments!"
  Reference
    This should be on its own page.
    You should explain the term, "type generator," by linking to
      http://www.boost.org/more/generic_programming.html#type_generator
    bullet 5: Change "denotes" to "denote"
    para 2: Change the first sentence to read, "It is worth
      noting that some functionality requires partial template
      specialization.  In particular, full array support requires
      it, but boost::array is typically a better choice."
    para 2: Change "special" to "specially" in the last sentence
    ExternalCollectionConcept
      para 1: Change the second paragraph to read, "Even
        though...boost, additions to the library, to support new
        types, can be done in any namespace."
    Synopsis
      comment, between result_iterator_of and begin(): Change
        "funtions" to "functions"
    Semantics
      para 1: Change "an iterator which default" to "an iterator
        type for which default"
      table 1, row 3, col 2: Shouldn't "P::first_type" be "const
        P::first_type?"  I think this is addressed by a FAQ.
      table 1, row 3, col 2: Shouldn't "I" be "const I?"
      table 2, row 2, col 3: Lines wrap so there is no clear
        demarcation between the four effects; perhaps you should
        use an unordered list (bullets) in this column
      para 2: Change "behaves differently" to "behave
        differently"
      para 2: I presume "these cases" refers to size() and end(),
        but that isn't clear.  If that's your intent, then I
        suggest, "Note that the null pointer is allowed as an
        argument to size() and end()."  Otherwise, you need to
        clarify the sentence.
    Portability
      para 2: Change "rely of" to "rely on"
      para 2: "...one needs to supply...f it is required" is
        confusing.  Is it required?  When?
  FAQ
    FAQ 1: The answer isn't satisfying.  Why is it not possible
      in general?  Why is it not desirable in general?  The last
      sentence is, I think, meant to be a workaround for the
      missing functionality, but isn't clear on that point.
    FAQ 2: Change to read, "Why doesn't the library support type
      <I>your favorite type</I> or function <I>your favorite
      function</I>?"

________________
external_concepts.html
  How about "extrinsic" rather than "external?"
  Isn't "namespace scope" more appropriate than "free-standing"
    when describing the functions?
  para 2: Change "which provides the exact" to "which provide the
    exact"
  para 2: Change "the new requirements constitutes" to "the new
    requirements constitute"
  para 2: The second to last sentence is lacking.  I suggest
    changing it to, "We say that a type, for which the
    free-standing functions and typedefs adapt the type to
    fulfill the requirements of an external concept, conforms to
    an external concept or that is is a model of an external
    concept."
  para 3: Add a heading: "Translating between External Concepts
    and Concepts"
  para 3: Change "qulifiers" to "qualifiers"
  para 3: Change "has the name as the nested typedef with _of
    appended" to "has the same name as the nested typedef plus a
    suffix of _of."
  Example
    para 1: Change "follwing" to "following"
    para 3: Change "type-generators exists" to "type-generators
      exist"

-- 
Rob Stewart                           stewart_at_[hidden]
Software Engineer                     http://www.sig.com
Susquehanna International Group, LLP  using std::disclaimer;