[boost] Re: [review] Formal review of the SmartContainerlibrarystarts today

From: Pavel Vozenilek (pavel_vozenilek_at_[hidden])
Date: 2004-09-26 16:41:16

Hello Thorsten,

> |  - small readable code examples
>
> ok, what do you find particular non-readable about all the examples in the
> example section?
>
They should fit into 1/2 of page whenever possible.

 _____________________________________________________
> | 2. docs: the link to license file may be better local.
>
> ok, why?
>
for those w/o inet access all the time

 _____________________________________________________
> | 7. docs, Introduction:  Assignable and
> |    Copy Constructible could be hyperlinked.
>
> ok, don't people know this?
>
Boost authors for sure. Ther are also beginners.

| _____________________________________________________
> | 14 Clonable concept: would it make any sense to provide
> |    support for placement new/delete also?
>
> I can't answer that question since I have never had the need to use it. I
hope
> other reviwers can anser it.
>
This feature can be likely accomplished when object
factories get supported

_____________________________________________________
> | 15. Clonable concept implementation:
> |     shouldn't there be try/catch in:
> |
> |     template< class T >
> |     void delete_clone( const T* t )
> |     {
> |         checked_delete( r );
> |     }
> |
> |    to enforce the constraints?
> |    At least it would be useful to have it
> |    in debug mode to report violation.
>
> but then again, shouldn't it then be in checked_delete()? In some
> sense I feel it
> beyond a library to cope with "fools" :-)
>
checked_delete accepts 0. smart containers cannot.

 _____________________________________________________
> | 20. Clonable concept: is there any requirement on
> |     *a_object == *a_cloned_object?
>
> I guess the requirement could be tightened to say as above iff
> there is an operator==(). I'm not sure its useful to say so, so why add
it?
>
Probably it isn't useful.

| _____________________________________________________
> | 21. Clonable concept: is there anything similar to
> |      std::nothrow?
> |
> |     Something as allocate_clone<std::nothrow>(p);
>
> If people want it there could be. However, we don't want 0 pointers in the
> containers, so
> what would be the point of it?
>
You are right.

| _____________________________________________________
> | 22. Clonable concept: would it be possible to add
> |     support for object factories? Such thing may
> |     be useful e.g. for COM or CORBA objects.
>
> yes, how do they look like?
>
I'll think about details.

> |    These exceptions should be declared in
> |    <boost/smart_containers_fwd.hpp>.
>
> what for?
>
it is quite common.

#include <...fwd> // no need for full declaration or to search where
exceptions are

try {
   some_func_using_some_smart_container()
} catch(smart_container_base_exception&) {
}

 _____________________________________________________
> | 65. details/scoped_deleter.hpp: there seems no need
> |     for scoped_array here (only to bloat executable
> |     and slower compilation).
>
> hm...I need to manage a resource and I prefer not to do so manually.
> I canøt imagine that scoped_array<> afffects the executable at all.
>
There could be performance advantage not to use scoped_array.
And it is one line in destructor.

> | _____________________________________________________
> | 67. It would be nice to support systems without
> |     enabled exceptions (BOOST_NO_EXCEPTIONS).
>
> this is harder than it seems because I would have to check return values
for
> 0, right?
>
I think no. Failed allocation should terminate application
in such case.

> | _____________________________________________________
> | 68.
> |
> | Likewise in ptr_map_adapter.hpp:
> |
> |         typedef BOOST_DEDUCED_TYPENAME
> |             smart_container::detail::map_iterator<
> |                        BOOST_DEDUCED_TYPENAME
Map::const_reverse_iterator,
> |                        BOOST_DEDUCED_TYPENAME Map::key_type,
> |                        const value_type>
> |                      const_reverse_iterator;
> |
> |
> |     I have problem
> | to find what
> |     is end and what
> |         is beginning here
>
> what do you suggest?
>
Alexandrescu's style?

      typedef BOOST_DEDUCED_TYPENAME
      smart_container::detail::map_iterator
      <
          BOOST_DEDUCED_TYPENAME Map::const_reverse_iterator,
          BOOST_DEDUCED_TYPENAME Map::key_type,
          const value_type
     > const_reverse_iterator;

> Thanks for your review. Hair-raising as ususal :-)
>
One more question:

 - if I have polymorphic hierarchy, will the allocate_clone(T*)
   clone the dynamic type of class or its static type? I ask because
   there's semifinished library (polymorphic_map) in Files section
   that could be used for such purpose.

/Pavel