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

From: Thorsten Ottosen (nesotto_at_[hidden])
Date: 2004-05-04 19:29:31

Hi Rob.

Thanks for you review.

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

ok. Is that the correct term?

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

is that a requirement? If so, I will change it.

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

Some like it...others don't. I guess I will use the bigger headers mostly.

| Wrong comment on std::pair specialization of result_iterator_of.

thanks.

| 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.

True. But the implementation is so simple, that it hardly helps
to put the code thruogh another layer of templates. Doing so might
give longer compiles. And some people care about minimal headers.

| 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.

I guess it could. My only problem is that I don't know how portable
this is. AFAIK, detecting nested typedefs only works with Comeau.

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

yes. maybe. I don't find it very useful. But if people do,  I can put
BOOST_COMPILE_TIME_ARRAY_SIZE() macro in detail/array_size.hpp.

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

yes.

| 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.

IMO the introduction is the motivation. what would you like to see in a motivation section?

| 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.

Wouldn't it only duplicate stuff in the two links to CollectionCocept and ExternalConcepts?

| Using the namespace
| scope functions is central to the library, so it should be stated
| early and clearly.

ok. I should add something about extending the lib + how to rely on ADL.

| Documentation Comments

Thanks. I will look into them when ) update the docs.

br

Thorsten