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