Skecher refactor of PropertyGeometryList

[abdullah]

I have started locally a refactor of PropertyGeometryList to use smart pointers (std::shared_ptr) instead of regular C pointers. This has become a necessity as we need to keep pointers to the same resource at different places, where often the lifetime of the resource is not properly manageable anymore with regular pointers.

So I am looking into how to offer in the interface shared and weak pointers, as well as looking into all the code that currently uses regular C pointers.

Given that this property is spread over the Sketcher WB, I would appreciate if no reformatting of existing files is done. Of course, regular PRs continue to be merged (there is no “freeze” for this reason).

For work reasons, I will not be available until 17 April. I will try catch-up with PRs and hopefully finish the refactor then.

[wmayer]

So I am looking into how to offer in the interface shared and weak pointers

I suggest to use shared_ptr internally and all methods to access a geometry item should use a weak pointer. This is to avoid that client code could prevent a geometry from being deleted. Especially for Python wrappers this is needed because there it can happen that somewhere a reference is kept and this we can hardly control.

Given that this property is spread over the Sketcher WB, I would appreciate if no reformatting of existing files is done. Of course, regular PRs continue to be merged (there is no “freeze” for this reason).

In a PR I am moving automatic SIGNAL/SLOT connections to manual connections and in sketcher code eight files are affected. But the changes are rather small and shouldn’t cause any headache.

[andrecaldas]

I have started locally a refactor of PropertyGeometryList to use smart pointers (std::shared_ptr) instead of regular C pointers. This has become a necessity as we need to keep pointers to the same resource at different places, where often the lifetime of the resource is not properly manageable anymore with regular pointers.

Love the idea!!!

I suggest to use shared_ptr internally and all methods to access a geometry item should use a weak pointer. This is to avoid that client code could prevent a geometry from being deleted. Especially for Python wrappers this is needed because there it can happen that somewhere a reference is kept and this we can hardly control.

Wow!!! I didn’t now about weak_ptr. I am reading the cppreference, right now.

Looks awesome. Life after C++11 is so much better!!! The weak_ptr is very self-documenting. As far as I understood, the “client code” will be very aware the it needs to be prepared to handle the case where the pointed object has already been destroyed. And also, it makes very easy for the client to know of this fact.

[abdullah]

In a PR I am moving automatic SIGNAL/SLOT connections to manual connections and in sketcher code eight files are affected. But the changes are rather small and shouldn’t cause any headache.

That’s all right.

I suggest to use shared_ptr internally and all methods to access a geometry item should use a weak pointer. This is to avoid that client code could prevent a geometry from being deleted. Especially for Python wrappers this is needed because there it can happen that somewhere a reference is kept and this we can hardly control.

There is definitely a use case for weak pointers. I have one at the model-view I am coding for a new TaskSketcherElements, where if the resource has been deleted I need to handle it differently (as it makes no sense to update the model with information about a geometry that no longer exists).

On the other side, having only the possibility to retrieve weak pointers from PropertyGeometryList may not be very convenient (or even efficient).

PropertyGeometryList stores internally a std::vector<std::shared_ptr>. This means that to provide weak pointers, one has to create a new array of type

std::vector<std::weak_ptr<Geometry>>

and populate it in a loop with the shared pointers (we do something similar for geometry extensions in Part::Geometry::getExtensions).

In many operations (using the legacy approach), the first thing to do is to lock the weak pointers to make the shared_ptrs, just to create a new list, modify some geometries (by making clones of the std::shared_ptrs first), and then modify the property by transferring the ownership of the vector of shared_ptrs back to PropertyGeometryList. Which idiomatically should be something like (transfer ownership of a vector of smart shared pointers):

void setValues(std::vector<std::shared_ptr<Geometry>> &&);

These operations I am referring to guarantee that the lifetime of no pointer will be deleted during it (they grab the property, modify it creating new pointers where necessary, and they set the property with the modified version). So perhaps it would make sense to have some function similar to this one:

const std::vector<std::shared_ptr<Geometry>> &getValues() const {
        return _lValueList;
    }

in addition to one like this:

const std::vector<std::weak_ptr<Geometry>> getValuesAsWeakPtr() const

Alternatively (or in addition), I am considering providing a different interface for these operations:

geometry.setValuesSequentially(int index, std::shared_ptr<Geometry> && newgeometry pointer)

editlock<geometry>; //  blocks hasSetValue until it goes out of scope
geometry.setValuesSequentially(index1, newgeometrypointer);
geometry.setValuesSequentially(index2, newgeometrypointer);

This may make the property way easier to use in client code, as it avoids having to create a new list and track which pointers to directly copy and which ones to clone and then assigning the whole list even if often many pointers are unchanged…

Changes to PropertyGeometryList usually also call for changes to:

1. Part::Geometry::clone() and Part::Geometry::copy() so that they return smart pointers too.
2. GeometryFacade (to take a smart pointer instead of a C pointer)
3. GeomList and GeomListFacade (to take a smart pointer instead of a C pointer)
4. About to all methods in SketchObject.
5. Sketcher::Sketch (the solver interface).

These are so core that they recursively extend to almost every other class in Sketcher WB, making it a huge refactor.

I have started two refactors to throw away just to see the problems and try to get a better solution.

That’s why I am also considering to keep different interfaces, including a regular pointer interface, at least temporarily, to support legacy code and split changes into different phases. Then when everything is ported, the C pointer interfaces may be removed for good.

Of course, I also see the risks associated to different interfaces and how they may be abused (for example for retaining a copy of a pointer and preventing it from being released).

All this are just ideas trying to find the way ™.

I will now have over one week to think about it. Then when I am back hopefully I will have a clearer mind. Of course, if you see further, leave a comment here and I take it as input.

[andrecaldas]

const std::vector<std::weak_ptr<Geometry>> getValuesAsWeakPtr() const

The weak ptr version should be the one called getValues. Or a copy of the vector. That would automatically increase the shared ptr count. I think there is no need to return a reference to the original data, because if performance is a concern, then I guess we should use iterators.

const std::vector<std::shared_ptr<Geometry>> &getValues() const {
        return _lValueList;
    }

Maybe one should get an iterator, instead.

If not, for the const reference, it might make more sense (I don’t really know what I am talking about) to “reinterprete cast” to make it const std::vector<const std::shared_ptr>. With iterators you can have a const_iterator.

[andrecaldas]

So I am looking into how to offer in the interface shared and weak pointers, as well as looking into all the code that currently uses regular C pointers.

I don’t really know anything about shared_ptr, but what I have read those last days… forgive me if I am saying anything silly.

I started to write some code… and I expect that in a near future (hopefully), the public interfaces will handle me a weak_ptr. But I don’t want to wait for it to happen. So, I am doing it like this (don’t know if it is correct):

class MyClass
{
public:
  MyClass(std::weak_ptr<XXX>&& pointer) : pointer(pointer) {}
  MyClass(XXX* _pointer) // Deprecated.
    : _pointer(_pointer, [](auto /*p*/){}) // A deleter that does not delete. :-)
    , pointer(_pointer) {}
  
private:
  std::shared_ptr<XXX> _pointer; // Temporary hack.
  std::weak_ptr<XXX> pointer;
};

[andrecaldas]

So I am looking into how to offer in the interface shared and weak pointers, as well as looking into all the code that currently uses regular C pointers.

abdullah and wmayer,

I am doing some work with shared pointers and I realized there are at least two cases:

1. Someone wants to get a pointer just to do some work and release it.
2. Someone wants to store the pointer.

In the second case, of course, this person should use a weak_ptr.

In the first case, however, the developer needs to be aware that not only the weak_ptr has to be converted to a shared_ptr. But also, that the locked resource might be gone, and the validity of the “locked” resource needs to be assured.

In the case of a gone resource, the developer needs to treat this “exception”. Now, I wonder what would be a nice way to deal with resources that were already gone.

Should we have a getLockedResource(…), that returns a shared_ptr? In this case, the documentation needs to inform the developer that s/he is not supposed to store a “locked resource”. The good point is that getLockedResource(…) could handle invalid pointers and throw an exception.

Or shall we always give the user a weak_ptr and let her/him the job of checking if(!resource) {throw an exception or deal with it here}?

The answer depends on how those cases should usually be dealt with.

[andrecaldas]

Well… this is all not a real problem if there are guarantees that whoever is passing a weak_ptr does hold a shared_ptr… and there is no asynchronous stuff going on. But this would be the case where a naked pointer would be enough.