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