On 2/6/06, Fred Bertsch <fred.bertsch@gmail.com> wrote: Please always state in your review, whether you think the library should be
accepted as a Boost library!
An enthusiastic YES. Additionally please consider giving feedback on the following general
topics:
- What is your evaluation of the design?
The design is very good.
- What is your evaluation of the implementation?
I agree with others that there should be RAII versions of the constructors of various classes like shared_memory and named_shared_object that throw if they fail. Another concern, addressed clearly in the documentation, is the necessity of reproducing the synchronization primitives of Boost.Thread and the xtime class in Shmem. This is yet another case where a library that tries to be thread-aware, without necessarily being multithreaded itself, has had to do this. From what I can gather from some grepping, Boost.Pool(pool/detail/mutex.hpp) and Boost.Regex (regex/pending/static_mutex.hpp) contains their own mutex implementation as well. Also, the proposed Logging library (which was pulled before the review was complete) and ASIO both contain some elemental synchronization classes that could be a more fundamental part of Boost. I think it is high time for the separation of the synchronization primitives of Boost.Thread into a separate library before they are duplicated again. This duplication has the follow-on issues of reduced portability for the library that reproduces them and the possibility of bugs being introduced. I'm also intrigued by the reproduction of many Standard Library containers due to implementation restrictions (under Current Limitations, "Problems with most STL implementations"). Ion, can you include a listing of platforms for which the Shmem versions of containers must be used and ones where the Standard Library containers can be used with the Shmem allocator? Are there any of the latter? A table would be quite useful here. For compilers with a reasonable hope of getting changes fed into the Standard Library implementation (e.g. gcc) it might be worthwhile to contact the maintainers about these limitations to see if they can be removed. - What is your evaluation of the documentation? Very good and quite complete. Some minor questions/quibbles:
From Current Limitations / "Be careful with static class members":
"Static members are not dangerous if they are just constant variables initialized when the process starts, but they don't change at all (for example, when used like enums)." I'm not sure I understand the second half of this sentence. What does it mean to use a static variable like an enum? Maybe I'm just being thick-headed. The Introduction states: "Shmem also offers the basic_string pseudo-container to use full-powered C++ strings in shared memory." But this class isn't documented. I see it in the header <boost/shmem/containers/string.hpp>. Actually it appears that none of the headers in the shmem/containers dir are mentioned explicitly in the documentation. They are touched on briefly in "Shmem and containers in shared memory", but this documentation might benefit from some expansion. - What is your evaluation of the potential usefulness of the library? I think it has great potential, especially if the memory segments can be made to grow (fingers crossed!). - Did you try to use the library? With what compiler? Did you have any
problems?
I didn't actually use the library other than to compile and run the tests w/gcc 3.3.4 on Linux. I did run into one error trying to compile the tests with a CVS version of Boost from this morning: "g++" -c -DBOOST_ALL_NO_LIB=1 -g -O0 -fno-inline -pthread -Wall -ftemplate-depth-255 -I"../../../bin/boost/libs/shmem/test" -I "/home/nbde52d/src/boost-regression/boost" -o "../../../bin/boost/libs/shmem/test/private_node_allocator_test.test/gcc/debug/threading-multi/private_node_allocator_test.o" "private_node_allocator_test.cpp" "/usr/bin/objcopy" --set-section-flags .debug_str=contents,debug "../../../bin/boost/libs/shmem/test/private_node_allocator_test.test/gcc/debug/threading-multi/private_node_allocator_test.o" /home/nbde52d/src/boost-regression/boost/boost/shmem/allocators/private_node_allocator.hpp: In function `void boost::shmem::swap(boost::shmem::private_node_allocator<boost::shmem::detail::shmem_list_node<priv_node_allocator_t>, 64, boost::shmem::detail::segment_manager<wchar_t, boost::shmem::simple_seq_fit<boost::shmem::shared_mutex_family, boost::shmem::offset_ptr<void, boost::shmem::offset_1_null_ptr> >, boost::shmem::flat_map_index> >&, boost::shmem::private_node_allocator<boost::shmem::detail::shmem_list_node<priv_node_allocator_t>, 64, boost::shmem::detail::segment_manager<wchar_t, boost::shmem::simple_seq_fit<boost::shmem::shared_mutex_family, boost::shmem::offset_ptr<void, boost::shmem::offset_1_null_ptr> >, boost::shmem::flat_map_index> >&)': /home/nbde52d/src/boost-regression/boost/boost/shmem/detail/utilities.hpp:45: instantiated from `void boost::shmem::detail::swap(T&, T&) [with T = boost::shmem::private_node_allocator<boost::shmem::detail::shmem_list_node<priv_node_allocator_t>, 64, boost::shmem::detail::segment_manager<wchar_t, boost::shmem::simple_seq_fit<boost::shmem::shared_mutex_family, boost::shmem::offset_ptr<void, boost::shmem::offset_1_null_ptr> >, boost::shmem::flat_map_index> >]' /home/nbde52d/src/boost-regression/boost/boost/shmem/containers/list.hpp:317: instantiated from `void boost::shmem::detail::shmem_list_alloc<T, A, true>::swap(boost::shmem::detail::shmem_list_alloc<T, A, true>&) [with T = int, A = priv_node_allocator_t]' /home/nbde52d/src/boost-regression/boost/boost/shmem/containers/list.hpp:598: instantiated from `void boost::shmem::list<T, A>::swap(boost::shmem::list<T, A>&) [with T = int, A = priv_node_allocator_t]' /home/nbde52d/src/boost-regression/boost/boost/shmem/containers/list.hpp:792: instantiated from `void boost::shmem::list<T, A>::sort(StrictWeakOrdering) [with StrictWeakOrdering = boost::shmem::list<int, priv_node_allocator_t>::value_less, T = int, A = priv_node_allocator_t]' /home/nbde52d/src/boost-regression/boost/boost/shmem/containers/list.hpp:774: instantiated from `void boost::shmem::list<T, A>::sort() [with T = int, A = priv_node_allocator_t]' private_node_allocator_test.cpp:95: instantiated from here /home/nbde52d/src/boost-regression/boost/boost/shmem/allocators/private_node_allocator.hpp:198: error: call of overloaded `swap( boost::shmem::offset_ptr<boost::shmem::detail::segment_manager<wchar_t, boost::shmem::simple_seq_fit<boost::shmem::shared_mutex_family, boost::shmem::offset_ptr<void, boost::shmem::offset_1_null_ptr> >, boost::shmem::flat_map_index>, boost::shmem::offset_1_null_ptr>&, boost::shmem::offset_ptr<boost::shmem::detail::segment_manager<wchar_t, boost::shmem::simple_seq_fit<boost::shmem::shared_mutex_family, boost::shmem::offset_ptr<void, boost::shmem::offset_1_null_ptr> >, boost::shmem::flat_map_index>, boost::shmem::offset_1_null_ptr>&)' is ambiguous /home/ietdev/tools/linux-i686/gcc-3.3.4/include/c++/3.3.4/bits/stl_algobase.h:121: error: candidates are: void std::swap(_Tp&, _Tp&) [with _Tp = boost::shmem::offset_ptr<boost::shmem::detail::segment_manager<wchar_t, boost::shmem::simple_seq_fit<boost::shmem::shared_mutex_family, boost::shmem::offset_ptr<void, boost::shmem::offset_1_null_ptr> >, boost::shmem::flat_map_index>, boost::shmem::offset_1_null_ptr>] /home/nbde52d/src/boost-regression/boost/boost/shmem/detail/utilities.hpp:43: error: void boost::shmem::detail::swap(T&, T&) [with T = boost::shmem::offset_ptr<boost::shmem::detail::segment_manager<wchar_t, boost::shmem::simple_seq_fit<boost::shmem::shared_mutex_family, boost::shmem::offset_ptr<void, boost::shmem::offset_1_null_ptr> >, boost::shmem::flat_map_index>, boost::shmem::offset_1_null_ptr>] - How much effort did you put into your evaluation? A glance? A quick
reading? In-depth study?
I've read the documentation going back to the Ion's version 0.3 and re-read it from the Review Materials. Overall, a few hours of reading and thinking. - Are you knowledgeable about the problem domain? A little bit. -- Caleb Epstein caleb dot epstein at gmail dot com