Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove OwnPtr #16138

Open
wants to merge 22 commits into
base: master
Choose a base branch
from
Open

remove OwnPtr #16138

wants to merge 22 commits into from

Conversation

IamPete1
Copy link
Member

This removes the use of OwnPtr, this simplifies the code and paves the way for scripting i2c. This also removes the need for std::move().

#14276 (comment)

This has not been tested extensively and the self-deleting functionality of OwnPtr has not been replicated. Possibly some deletes will have to be added.

@IamPete1 IamPete1 force-pushed the own_ptr branch 5 times, most recently from b235740 to 4977609 Compare December 26, 2020 20:52
@lucasdemarchi
Copy link
Contributor

There is a reason why it exists: to manage ownership of data and moving the ownership from on component to the other. It would be ok to replace it with something else (particularly std::unique_ptr if we don't have the problems we had with toolchain a few years back)… but just removing it, doesn't feel the right approach.

@lucasdemarchi
Copy link
Contributor

Also the explicit use of std::move means we are not implicitly copying the object. This avoids unintended behavior and allows the compiler to do return value optimization in more places.

@IamPete1
Copy link
Member Author

For me to took me quite some time to get my head round OwnPtr (i'm not sure I have 100%), for me it adds more confusion than it removes. After all in AP we very rarely actually move a object more than once.

The use case from my end is to get scripting I2C working (and SPI in the future), I was not able to copy the OwnPtr object into the lua heap as userdata.

@lucasdemarchi
Copy link
Contributor

OwnPtr is basically a container that stores a pointer. It's basically a lightweight/smaller version of std::uniq_ptr. If you read the doc for uniq_ptr you should get a better understanding of OwnPtr, even if for removing it.

For lua, unless you are passing ownership, you should/could actually pass the raw pointer.

@tridge
Copy link
Contributor

tridge commented Dec 28, 2020

a problem with OwnPtr is it is based on a false premise. We do have cases where we want two drivers to access to the same device, eg. lsm303d which is a mag and accel, and mavlink DevOp operations.
I think the cost in developer confusion is higher than what we gain

@lucasdemarchi
Copy link
Contributor

It's not based on a false premise because it's orthogonal to multiple drivers accessing the devices. The only thing that it manages is the ownership of the pointer it contains and if the device is being accessed by more drivers plays no hole in that.

My main worry is that removing it without understanding what it is doing will create bugs. The I2C/SPI device manager in the Linux hal was built around this concept of passing ownership and automatically deregistering when the object is deleted (or the OwnPtr goes out of context).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants