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

From: Thorsten Ottosen (nesotto_at_[hidden])
Date: 2004-04-29 07:53:08

Hi Pavel,

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.

agree.

| 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.
libs/collection_traits/index.html.

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

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
in
different namespace as long as client code uses *unqualified* calls to the
functions.

this means that I need to explain how to add support for more types. Eg,
begin()
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
|    examples).

yes.
| _______________________________________________________
| 6. examples in docs: each example should have at the end
|    expected output as comment.

yes.

| _______________________________________________________
| 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
namespace
boost, so I changed it. but I'm sure the review manager will record your
opinion.

| _______________________________________________________
| 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 ) )
    ...
else
  ;

| _______________________________________________________
| 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?
|
|    ptr_containers?

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.
So
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
container requirements.

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

--------------Abbreviations-----------
SC = std container
T    = the type used in arrays
P   = std::pair
etc

help

?

| _______________________________________________________
| 11. examples/iterator.cpp:
|
|   const char* this_file = "iterator.cpp";
|
| ==>>
|
|   const char* this_file = __FILE__;
|
| Some comments in the file would be useful.

ok.

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

ok.

| _______________________________________________________
| 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
Pavol needs
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
|    Traits?

so you're describing a search facility for [] containers. it will work a bit
strange
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[0] = ..
i[1] = ..

or

rbegin()[0] = ...;
rbegin()[1] = ...;

Maybe that could justify having reverse_iterator_of<> and
const_reverse_iterator_of<>
+ 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.

yep.

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

std::size_t

| _______________________________________________________
| 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
something like

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[0])) \

seems to be far more portable.

| The file sizer may be moved into details and
| names size_calculator.hpp or so.

yeah.

| _______________________________________________________
| 23. headers: in guard like
|
| #if defined(_MSC_VER) && (_MSC_VER >= 1020)
| # pragma once
| #endif
|
| 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).

ok

| _______________________________________________________
| 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
nothing
else. Unicode charactors is not supported right now. I have about zero
understanding about
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[0] != end; )
|                 ++s;
|
| strlen/wstrlen may be better optimized.

ok. how?

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

ok.

| GCC allows zero size arrays.
|
 does not?

| _______________________________________________________
| 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
|
|                         char,
|                         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..
detail/function.hpp
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/

ok.

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

_impl ?

br

Thorsten