Re: [eigen] Memory leaks

On Wed, Apr 15, 2015 at 11:14 AM, Christoph Hertzberg <chtz@xxxxxxxxxxxxxxxxxxxxxxxx> wrote:

Am 15.04.2015 um 09:32 schrieb Gael Guennebaud:

On Tue, Apr 14, 2015 at 5:09 PM, Christoph Hertzberg <
chtz@xxxxxxxxxxxxxxxxxxxxxxxx> wrote:

here Base is called with rows and cols of `other`, even though `other`
needs to be transposed. Base then does not assert that cols==1.

yes, we could add internal assertions for that, and also to check that size
== rows*cols in DenseStorage.

Yes, we should (after fixing this issue).

Is there a reason to call Base(size, rows, cols) at all? The call to
Base::_set_noalias(other);
should do all appropriate resizing, does it not?

I've removed that line and my test-case passes now. I'm running the
test-suite now to see if this has any unforeseen side-effects.

yes, this seems to be the simplest solution.

While the test-suite ran without (new) failures, I found a possible reason for calling the Base constructor. When compiling with
EIGEN_NO_AUTOMATIC_RESIZING
the Base::_set_noalias(other) call would assert (which it should not for the constructor). Adding a call to resizeLike(other) -- not _resize_to_match -- should do the job.

yes, we could call resizeLike either from the ctor or _set_noalias making sure that _set_noalias will always be called from ctors only.

Two other things:
The ReturnByValue constructor does not implicitly transpose the result if required, is this intended? Implementing this would require a few lines of additional code in the assignment logic.

hm, have to check first whether operator=(ReturnByValue) does implicit transposition without extra copy.

The constructors of Array and Matrix are almost identical, except that Array has an ArrayBase and Matrix has a MatrixBase constructor. Both have an EigenBase constructor (which, however, calls operator= instead of _set_noalias).
Couldn't we delegate all constructors to PlainObjectBase, and implementing for Array and Matrix only two constructors each:

Array(const Array& other) : Base(other.derived()) {}
Array(const EigenBase<OtherDerived>& other) : Base(other.derived()) {}

Or is there a reason that, e.g., Matrix(const ArrayBase<..>&) shall be handled differently than Array(const ArrayBase<..>&) ?
In PlainObjectBase both could be handled by a DenseBase<..>& constructor (calling _set_noalias(other), instead of operator=).

I don't see pitfalls, so the lesser number of boilerplate code, the better ;)

thanks for looking at this mess. In Eigen 4(-ever) automatic transposition should be abandoned ;)

gael

Christoph

--
----------------------------------------------
Dipl.-Inf., Dipl.-Math. Christoph Hertzberg
Cartesium 0.049
Universität Bremen
Enrique-Schmidt-Straße 5
28359 Bremen

Tel: +49 (421) 218-64252
----------------------------------------------