|
|
Ublas : |
From: Vardan Akopian (vakopian_at_[hidden])
Date: 2005-08-25 13:05:12
Hi Michael,
I was (and still am) heavily using the shallow_array_adaptor class. I
have not upgraded yet to 1.33.0 and haven't gotten a cvs update for
about 2 months, so I can't report any factual problems, but these
changes worry me. Comments inline.
On 8/20/05, Michael Stevens <mail_at_[hidden]> wrote:
> I have made some changes to the implementation of the array adaptors that were
> defined in "storage.hpp"
>
> Previously 'array_adaptor' and 'shallow_array_adaptor' were defined. They had
> two major problems:
> a) They were never documented or tested
Absolutely right, it was a major pain to find out and (try to) correctly use it.
> b) In order to make them Copy Construtable they would allocate their own
> storage.
This is not true for shallow_array_adaptor (it's true for
array_adaptor). The shallow_array_adaptor would just copy the internal
data_ member, which is a boost::shared_array.
> ii) It is very easy to accidentally perform operations which cause
> construction and then expected changes to the adapted array would not happen.
Not true for the shallow_array_adaptor for the above reasons.
>
> In new version the two existing classes have been replaces by the simpler
> 'carray_adaptor'. This is now fully documented. The changes are committed to
> Boost HEAD so please take a look at the documentation.
Thank you very much for the documentation.
I took a look at the new class, and it looks like you enforce that
carray_adaptor is not copy constructable. Doesn't this effectively
render it useless as an adaptor for any matrix class? Many (all?)
matrix classes copy construct their internal array when they are copy
constructed (or is this changed in 1.33.0 too?).
I also don't see any reason not to allow copy construction for
carray_adaptor. Why not just copy the pointer (as
shallow_array_adaptor was in effect doing)? And to make it consistent
with this (and for another reason below), I think the assignment
operator should behave the same way, i.e. copy the pointer, instead of
resizing and copying the data as it does right now.
The way it's written, I don't think carray_adaptor would even compile,
since it's calling resize_internal(), but it has no such member
function. Am I missing something?
Isn't the idea of carray_adaptor (and shallow_array_adaptor) to use an
externally allocated memory for storage without doing any internal
allocation? Doesn't this mean that it is not (and should not) be
responsible for deallocating the storage? Now if we write resize()
and/or assignment operator the way they are written for carray_adaptor
and shallow_array_adaptor, I think we start owning the storage and
therefore are responsible for its deallocation, which neither class's
destructor does.
In conclusion, here is how I would change the current carray_adaptor
1) allow copy constructor by just copying the internal pointer to the storage.
2) remove all the resize functions (or declare then as private and not
implemented, or use whatever technique boost has for this type of
control). carray_adaptor by its nature should not be resizable.
3) change the assignment operator to only copy the pointer to the
storage (and the size) instead of copying the data.
4) add test cases for carray_adaptor (are there any already?), since
it's now an officially documented class ;-).
Thanks.
-Vardan