Fixing issue #779

[rolanddenis]

PR Description

The issue #779 comes from the fact that ImageContainerBySTLMap stores his domain by pointer.
But readers create domain on the stack before constructing images so that the pointer is invalid as soon as the reader ends.

Therefore, a solution is to store the domain by value, like other image containers.
But, doing so (like in this PR), leads to compilation errors when the domain is not assignable. It is the case when using a DigitalSetDomain, like in tests/geometry/volumes/distance/testPowerMap.

In fact, this problem arises for all CTrivialConstImage models that store the domain by value, since it is a refinement of boost::Assignable.

Any suggestion ?

Checklist

• [N/A] Unit-test of your feature with Catch.
• [N/A] Doxygen documentation of the code completed (classes, methods, types, members...)
• [N/A] Documentation module page added or updated.
• New entry in the ChangeLog.md added.
• No warning raised in Debug cmake mode (otherwise, Travis C.I. will fail).
• All continuous integration tests pass (Travis & appveyor)

[phcerdan]

Not really aware of the details here, but what about using std::shared_ptr instead of raw pointers?

Passing by shared_ptr only when lifetime, or ownership is involved, and by raw pointer std::shared_ptr::get() to interact with non-owning functions/classes. cpp core guidelines

From boost::shared_ptr it meets the CopyConstructible, MoveConstructible, CopyAssignable and MoveAssignable requirements.

[dcoeurjo]

We do have smart pointers in DGtal (http://dgtal.org/doc/nightly/moduleCloneAndReference.html). This issue seems to be related to bad object lifetime management in this container.
I'll have a look but that's not a good idea to copy domain by value since they can be "heavy".

[rolanddenis]

I agree that std::shared_ptr (or similar) is the right choice if we choose to keep pointer to the domain.
The problem is that, at this time, not all the image containers use the same way to refer to the domain and thus, it is difficult to write a generic image importer.
So there is some solutions:

• every image container stores domain by (smart) pointer, but that need a lot of modifications in the code;
• every image container stores domain by value and we add a kind of domain proxy class that handles an underlying domain by (smart) pointer, for the cases when domains are heavy;
• a mixed solution where images that store his domain by pointer, feature 2 constructors : one that copy a domain into a new shared_ptr (to ensure compatibility with current code), and one that take directly a shared_ptr.

I suppose that we choose the third solution ? Perhaps with CountedPtr ?

[dcoeurjo]

Couldn't it be a DGtal::CountedPtr ?
just for consistency..

[rolanddenis]

Yes it can ! However CountedPtr has no conversion from CountedPtr<T> to CountedPtr<const T> that may be necessary here. I will think about adding it...

[dcoeurjo]

Sorry for being late on this PR. Let me start with some general comments:

• all other image containers should have their constructor in the form

ImageContainer(ConstAlias<Domain> aDomain);

and member class could, for instance (several other options are possible as members, see doc page), be:

const Domain * myDomain;

(most image containers have been created before Const/Alias stuff). Lifetime of the domain must be larger than the container lifetime (i.e.: no domains on the stack)

• Thanks to some previous PR, @rolanddenis , if the user does a Image image(Domain(a,b)) an warning should be raised, am I right ?
• Plenty of smart pointers are already available in DGtal, I would recommend to use them
• Heavy/light data management int DGtal is something we have discussed a lot and maybe the solution we have right now is not satisfactory.
• Would you suggest to have both
  • a constructor with copy of the domain
  • a constructor with an (const) alias of the domain
    for all containers?

I know that @elcerdo has very different point of view of the data management: constructors always copy the data and when the user don't want to have memory copies, he/she wraps the data into a lightweight facade (with exact same interface)... It is not was is currently implemented in DGtal but that's maybe part of the discussion.

(ping @JacquesOlivierLachaud )

[rolanddenis]

all other image containers should have their constructor in the form
ImageContainer(ConstAlias<Domain> aDomain);

OK but that means that the caller controls the lifetime of aDomain but it is not possible for, e.g., image readers that should otherwise returns the image and a smart pointer on the domain.

Thanks to some previous PR, @rolanddenis , if the user does a Image image(Domain(a,b)) an warning should be raised, am I right ?

No because ImageContainerBySTLMap is an exception among image containers: many of them (ImageContainerBySTLVector, ImageContainerByHashTree and mayby ImageContainerByITKImage ?) only handle HyperRectDomain domain type (that is not a heavy type) and store it by value.

Would you suggest to have both
- a constructor with copy of the domain
- a constructor with an (const) alias of the domain for all containers?

I don't think this is a good solution but I have choose it for compatibility reasons with current code. I don't like the idea that classes have to deal with their parameters size when they do not control their type (because of a template) and thus, I like @elcerdo solution even if it requires more works for the user.

A mixed solution may be what some linear algebra library uses (like Blitz++ if I'm not wrong): copy construction, like Array A(B), aliases data (A and B refers to the same array), while copy assignment really copies the data. But it is probably not what the user excepts from a copy constructor and I am not sure that this have no side effects...

[JacquesOlivierLachaud]

Just a silly C++11 question... The rvalue constructor of ConstAlias prevents the passing of temp objects.
And an image reader can return a CountedPtr and the ConstAlias should work fine.
Have I missed something ?

[rolanddenis]

After discussing with @JacquesOlivierLachaud, it seems that using Clone together with CowPtr is an appropriate choice in our case and that avoids using two different constructors.
In addition, I have modified CowPtr<T> to deactivate non-const methods when T is a constant type, in order to avoid unnecessary copies (NB: C++ calls non-const methods whenever it's possible, even if the expression does not modify the object. So, it would trigger a copy of the owned T if the instance of CowPtr<T> is non-const, even if T is constant.)

[JacquesOlivierLachaud]

Do you mind commenting a bit these lines ? (Like forces the compiler to choose the const version without copy whenever possible)

[rolanddenis]

I think it was a very comprehensible line, especially the typename U = T
Ok, I will add a comment 😉 ( in doxygen style or just for the devs ? )

[JacquesOlivierLachaud]

Just for devs I think.