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@sig.com Software Engineer http://www.sig.com Susquehanna International Group, LLP using std::disclaimer;