From: Thorsten Ottosen (nesotto_at_[hidden])
Date: 2004-04-29 07:53:08
Thanks for your comments. They are very thoughrough as usual.
| It would be useful to have many more examples in documentation,
| showing how to use it together with other Boost libraries.
Can you give an example of what you have in mind?
| 1. config.hpp:
[snip proper code]
I haven't studied the config very much, so thanks for your tutorial.
| The macros as BOOST_CT_NO_ARRAY_SUPPORT may be better
| named BOOST_COLLECTION_TRAITS_NO_ARRAY_SUPPORT.
| The CT thing is rather nonintuitive and may clash.
| BOOST_ARRAY_REF: should it be BOOST_COLLECTION_TRAITS_ARRAY_REF?
that seems reasonable.
| 2. docs: shouldn't there be index.html somewhere?
There should be one. Hartmut said it should be in the "root" ie.
| The *.php files are maybe not needed for users to see.
he he, no.
| 3. Collection.html:
| - "associated" should be explained
yeah. I guess what it says it that reference_type_of<T>::type need only be
convertible to T&. I am inclined to
remove that requirement because I fail to see why it is useful.
| - more models can be added
any container, since it is weaker than the container requirement
| - the email at the bottom may be better obfuscated
yeah :-) I would think it wouldn't work anymore.
| 4. external_concepts.html: the docs doesn't say _where_
| the free standing function should be, aboyt their
that should be added.
1. they can be anywhere since ADL will find the right version
2. It only makes sense for a public interface
| It should be also noted whether ADL lookup may kick in here
| or not.
it must ... which implies the implementation for different types can reside
different namespace as long as client code uses *unqualified* calls to the
this means that I need to explain how to add support for more types. Eg,
should be overloaded in namespace boost.
| 5. docs: maybe instead of showing 'raw' source code in examples
| syntax colorized HTML can be shown (both 'inline' and external
| 6. examples in docs: each example should have at the end
| expected output as comment.
| 7. iterator_of name: wouldn't just iterator be sufficient?
| Dtto size_type_of etc.
people have expressed concern for those short names when they were class in
boost, so I changed it. but I'm sure the review manager will record your
| 8. collection_traits.html, first example:
| typename boost::const_iterator_of<EC>::type found = find( c, value );
| *found = replacement;
| Will it always work with const iterator? And even if
| it will, shouldn't there be non-const iterator
| for peace in mind?
its a doc error. The actual example uses iterator_of<> as it should.
| What if nothing is found in the example?
| my_generic_replace ==>> my_replace_first
yeah, that should be fixed...
if( found != boost::end( c ) )
| 9. collection_traits.html, Reference section:
| are slist/hash_XXX/rope supported?
| Will boost::unordered_map/set be supported?
| boost::bitset, boost::array, spirit iterators?
| circular_buffer and maybe multi_index_container?
all standard compliant containers work without problems. That's the first
bullet in the reference section.
I guess I should stress that the container need not be part of the standard.
that include AFAICT
slist, robe, array, unordered_map/set, circular_buffer, ptr_container
bitsets don't have iterators, so they are not included.
multi_index_container? I can't remember if it fulfills the standard
spirit iterators...only if they inherit from std::iterator. (I can see my
iterator implementation is actually broken :-()
| 10. collection_traits.html, Semantics section, tables:
| it doesn't make sense to me what middle column contains.
ok, would a table with
SC = std container
T = the type used in arrays
P = std::pair
| 11. examples/iterator.cpp:
| const char* this_file = "iterator.cpp";
| const char* this_file = __FILE__;
| Some comments in the file would be useful.
| 12. collection_traits.html, Portability section:
| bcc6 ==>> BCB 6.4
I'm just curious, did you compile the test with that compiler?
| 13. collection_traits.html: I would definitely welcome
| many more small code snippets as examples.
ok. If any other reviewer wants to donate examples, please do.
| 14. collection_traits.html: there are many <br> at the
| end of HTML text. It feels as if something was cut
| from the file. maybe <br><i>EOF</i> can be there.
| 15. I hope string algorithms library will re-use this
| library before it gets into official Boost distribution.
I'm working with Pavol on that as we speak. But I think it is boost policy
not to include a library accepted too close to the new version, so maybe
to use an internal version anyway.
| 16. I once thought about having typedef boost::end
| and containers with overload of operator.
| It would allow to write:
| a_container[boost::end - 1]
| to access last element of container.
| a_container[boost::end - 2] would be one before
| the last one. Python has such feature.
| Is there some way to have such support in Container
so you're describing a search facility for  containers. it will work a bit
with map, wouldn't it? we can't use boost::end, but maybe boost::last. In
some sense it correponds to
typename reverse_iterator_of<T>::type i;
i = ..
i = ..
rbegin() = ...;
rbegin() = ...;
Maybe that could justify having reverse_iterator_of<> and
+ rbegin(), rend(). ?
| Maybe the library can provide function as
| boost::last(c, int n):
| int x = boost::last(vec, -2);
I guess you would want this to work with any collection. And it seems to be
another way to search. Anyway, would'nt we need to versions:
n_before_end( 2, vec );
n_after_begin( 2, vec );
| 17. collection_traits.hpp: the name of macro guard
| BOOST_CONTAINER_TRAITS_HPP should be changed.
| Dtto elsewhere.
| 18. value_type.hpp: comments as
| // pair
| are not particularly valuable.
| If necessary, it should be complete sentence.
they are mostly a way to seperate code in blocks.
| 20. value_type.hpp: do overloads for volatile make sense
| in this library? (I ask only for completeness and prefere
| not to clutter library with them.)
I'm not sure, but I think they don't make sense. What will happen is
that you will iterate over
volatile vector<int> vec;
using normal iterators. As usual--we don't have volatile_iterator
vector<T>::begin() volatile either.
| 21. value_type.hpp:
| shouldn't there be #include <iosfwd> or so
| for istream definition?
it should be enough with <iterator> when I fix the implementation.
| OTOH why is #include <cstddef> there? (Dtto elsewhere.)
| 22. sizer.hpp: is the
| template< std::size_t sz >
| class container_traits_size
| char give_size[sz];
| immune against padding/alignement?
no. It's not part of the official interface. The real version should be
char sizer( T (&array)[sz] )[sz]
but I decided not to include it. I failed to see it as particular important.
a macro a la
BOOST_ARRAY_SIZE( a ) \
sizeof((a)) / sizeof((a)) \
seems to be far more portable.
| The file sizer may be moved into details and
| names size_calculator.hpp or so.
| 23. headers: in guard like
| #if defined(_MSC_VER) && (_MSC_VER >= 1020)
| # pragma once
| sometimes value 1020 and sometimes value 1200 is used.
| Both are OK from practical point of view but
| it would feel better if the same.
I would like to know which one to use. I think the "right" one is 1200.
| 24. docs: there should be step-by-step example how to add
| support for new container (e.g. scattered array consisting
| of few subarrays).
| 25. end.hpp: what is #undef BOOST_END_IMPL doing here?
| Why such dangerous name is used?
to be removed.
| 26. testing Unicode string for being empty: I read
| somewhere Unicode files have two characters at the
| beginning to indicate endianess. I do not know if
| this applies to strings in memory as well.
| If so, then maybe Container Traits or String Algos
| should take care about this.
| (I could talk complete nonsense on this topic.)
my understanding is that both char and wchar_t has a trailing null and
else. Unicode charactors is not supported right now. I have about zero
unicode chars and what to expect for them...eg...will std::char_traits
exists for them etc.
| 27. functions.hpp: what is BOOST_STRING_TYPENAME and where
| does it come from?
A leftover from Pavol. To be removed.
| 28. implementation_help.hpp:
| strlen() should be used instead of
| for( ; s != end; )
| strlen/wstrlen may be better optimized.
| inline T* array_end( T BOOST_ARRAY_REF[sz],
| char_or_wchar_t_array_tag )
| return array + sz - 1;
| there should be static assert sz > 0.
| GCC allows zero size arrays.
| 29. sfinae.hpp: isn't the file name misleading?
you tell me.:-) It's basically sfinae for use in my type-traits.
| 30. result_iterator.hpp: what does the "give up"
| note there mean?
I can't implement that.
| 31. functions.hpp\: some commenst should be added.
| The source has 21 kB. The MPL expressions here
| are quite complex...
isn't that overkill?
| 32. functions.hpp: in list of types
| signed char,
| unsigned char,
| signed short,
| unsigned short,
| signed int,
| unsigned int,
| signed long,
| shouldn't floats and long long/__int64 be there as well?
hm...I think you're looking in a file that is depricated..
is not officially part of the dist.
| 33. each header should have short comment on the top what
| is it for, especially the ones in detail/
| 34. functions.hpp: is the reference e.g. in
| template< typename P >
| static result_iterator begin( P& p )
| return p;
| necessary? I assume 'p' is always pointer.
yeah. It should be changed.
| 35. naming conventions: maybe names as iterator_ should
| be replaced with something else. Underscore is easy
| to miss and is quite unusual.
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk