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

From: Pavel Vozenilek (pavel_vozenilek_at_[hidden])
Date: 2004-04-29 05:22:53

> The review of the Collection Traits library, written by Thorsten Ottosen
> starts today (April 28th, 2004) and runs for 10 days (until May 7th,
2004).
>
I think the library should become part of Boost.

It would be useful to have many more examples in documentation,
showing how to use it together with other Boost libraries.

I collected few notes (mostly nitpicks) on the code and docs, bellow.

/Pavel

_______________________________________________________
1. config.hpp:

#ifdef __BORLANDC__
#define BOOST_CT_DEDUCED_TYPENAME
#else
#define BOOST_CT_DEDUCED_TYPENAME BOOST_DEDUCED_TYPENAME
#endif

==>>

#include <boost/detail/workaround.hpp>

// BCB (6.4) has problems here with keyword 'typename' and these
// problems are not always fixed by simply using BOOST_DEDUCED_TYPENAME.
#if BOOST_WORKAROUND(__BORLANDC__, BOOST_TESTED_AT(0x564))
#  define BOOST_CT_DEDUCED_TYPENAME
#else
#  define BOOST_CT_DEDUCED_TYPENAME BOOST_DEDUCED_TYPENAME
#endif

#if _MSC_VER <= 1200
#define BOOST_CT_NO_ARRAY_SUPPORT
#endif

====>> (probably)

#if BOOST_WORKAROUND(BOOST_MSVC, <= 1200)
#  define BOOST_CT_NO_ARRAY_SUPPORT
#endif

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.

The macros here should have some description - some
of them look rather obscure, like BOOST_ARRAY_REF.

BOOST_ARRAY_REF: should it be BOOST_COLLECTION_TRAITS_ARRAY_REF?

_______________________________________________________
2. docs: shouldn't there be index.html somewhere?

Collection.html should be collection.html.

The *.php files are maybe not needed for users to see.

_______________________________________________________
3. Collection.html:

 - "associated" should be explained

 -  more models can be added

 - the email at the bottom may be better obfuscated

_______________________________________________________
4. external_concepts.html: the docs doesn't say _where_
   the free standing function should be, aboyt their
   visibility.

   It should be also noted whether ADL lookup may kick in here
   or not.

_______________________________________________________
5. docs: maybe instead of showing 'raw' source code in examples
   syntax colorized HTML can be shown (both 'inline' and external
   examples).

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

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

  What if nothing is found in the example?

  my_generic_replace ==>> my_replace_first

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

_______________________________________________________
10. collection_traits.html, Semantics section, tables:
    it doesn't make sense to me what middle column contains.

_______________________________________________________
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

_______________________________________________________
13. collection_traits.html: I would definitely welcome
    many more small code snippets as examples.

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

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

   Maybe the library can provide function as
   boost::last(c, int n):

   int x = boost::last(vec, -2);

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

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

_______________________________________________________
21. value_type.hpp:
       shouldn't there be #include <iosfwd> or so
    for istream definition?

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?

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

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

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

_______________________________________________________
27. functions.hpp: what is BOOST_STRING_TYPENAME and where
    does it come from?

_______________________________________________________
28. implementation_help.hpp:

strlen() should be used instead of
            for( ; s[0] != end; )
                ++s;

strlen/wstrlen may be better optimized.

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.
GCC allows zero size arrays.

_______________________________________________________
29. sfinae.hpp: isn't the file name misleading?

_______________________________________________________
30. result_iterator.hpp: what does the "give up"
    note there mean?

_______________________________________________________
31. functions.hpp\: some commenst should be added.
    The source has 21 kB. The MPL expressions here
    are quite complex...

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

_______________________________________________________
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.
If not, maybe call_traits<P>::valuue_type cvan be used.

_______________________________________________________
35. naming conventions: maybe names as iterator_ should
    be replaced with something else. Underscore is easy
    to miss and is quite unusual.

_______________________________________________________
EOF