Rethink about the using of unique_ptr in ParametersHandler library about bipedal-locomotion-framework HOT 4 CLOSED

Upvote for transforming every unique ptr into shared. I think it makes more sense given the application of the handler. I don't see why the ownership should be reserved. Feel free to close/edit my other PR to use shared pointers instead.

from bipedal-locomotion-framework.

I just realized that the issue is more complex than what I initially thought. The basic problem is that ownership is not clear. Having a unique_ptr to an object means that it is owned by a single actor. Hence, it is weird that when getting something out of it (getGroup) you get again a unique_ptr. It seems that someone can have unique possession of a part of a bigger object uniquely owned by someone else (this is not completely the case, but stick with me for a moment *).

It is like saying that you own a house and you only have the key of it, but someone else owns a room. So, what happens when I want to destroy the house? Also, that room should be destroyed right?

I was thinking that it may be more reasonable that getGroup returns a weak_ptr instead. This avoids problems in case some groups, for some reason, has a pointer to the outer group (this happens more often than expected, unfortunately). In addition, returning a weak_ptr makes it clear to whom the ownership belongs. If I decide to burn down the house, that poor guy who had the key of the room will find just a bunch of ashes (when try to lock an empty shared_ptr is returned).

So I tried.. and it did not work. Here you could say "I knew it" (*). Yes, because getGroup doesn't actually return a pointer to a part of the whole object, but rather a copy of it. In other words, the other guy own a key to an exact copy of the room that I just destroyed in my house. Hence, when returning a weak_ptr, I was always getting an empty pointer because the copy I was creating was being deallocated when going out of the scope of getGroup. In addition, returning a weak_ptr makes sense only if there is a shared_ptr somewhere actually owning the object.

Indeed, this is my fault 😁. It is a direct consequence of a review comment I made about the possibility to set parameters. In fact, before the parameter handler was owning a reference to a searchable (instead of a copy of it), hence when doing getGroup a new object was created with again a reference to the group in the initial object. In this case, the ownership problem was a bit different: neither I nor the other guy actually possessed the house, but a third entity with the original version of the searchable.

Here comes the other problem though. If I get a group and then I modify a parameter, this modification is not reflected to the original object owning the group. This was initially possible, but no more after the modifications necessary to have a set method.

So I guess that your problem can be solved via the following:

    A a1, a2;
    a1.initialize(params->getGroup("foo"));
    a2.initialize(params->getGroup("foo")); 

because you are actually getting two different copies of the same object. In this case it makes more sense to return a unique_ptr instead of a shared_ptr because you are actually generating whole new objects.

I don't really like this solution since I would like to have a pointer to a part of the whole, rather than a copy of it.

from bipedal-locomotion-framework.

@traversaro @S-Dafarra do you have any other ideas?

All what @S-Dafarra just wrote, plus:

Are you sure the issue is in the getGroup method signature, and not in the fact that the A::initialize takes a unique_ptr?

from bipedal-locomotion-framework.

I tried to fix the ownership and modifiability issues with this PR: #18

from bipedal-locomotion-framework.