[libcamera-devel] [PATCH 1/4] Documentation: coding_style: Add object ownership rules
Jacopo Mondi
jacopo at jmondi.org
Fri Jan 18 18:00:44 CET 2019
Hi Laurent,
On Fri, Jan 18, 2019 at 06:17:19PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Fri, Jan 18, 2019 at 03:56:08PM +0100, Jacopo Mondi wrote:
> > Hi Laurent,
> > thanks for doing this!
> >
> > I don't look picky here, but this is an important piece of
> > documentation, and I want to make sure I got it right first...
>
> This is exactly the kind of patch for which I expect picky reviews :-)
>
Happy to be of any help then :)
> > On Fri, Jan 18, 2019 at 01:59:13AM +0200, Laurent Pinchart wrote:
> > > Object ownership is a complex topic that can lead to many issues, from
> > > memory leak to crashes. Document the rules that libcamera enforces to
> > > make object ownership tracking explicit.
> > >
> > > This is a first version of the rules and is expected to be expanded as
> > > the library is developed.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > ---
> > > Documentation/coding-style.rst | 62 ++++++++++++++++++++++++++++++++++
> > > 1 file changed, 62 insertions(+)
> > >
> > > diff --git a/Documentation/coding-style.rst b/Documentation/coding-style.rst
> > > index f8d2fdfeda8e..f77325239bfa 100644
> > > --- a/Documentation/coding-style.rst
> > > +++ b/Documentation/coding-style.rst
> > > @@ -81,6 +81,68 @@ C++-11-specific features:
> > > overused.
> > > * Variadic class and function templates
> > >
> > > +Object Ownership
> > > +~~~~~~~~~~~~~~~~
> > > +
> > > +libcamera creates and destroys many objects at runtime, for both objects
> > > +internal to the library and objects exposed to the user. To guarantee proper
> > > +operation without use after free, double free or memory leaks, knowing who owns
> > > +each object at any time is crucial. The project has enacted a set of rules to
> > > +make object ownership tracking as explicit and fool-proof as possible.
> > > +
> > > +In the context of this section, the terms object and instance are used
> > > +interchangeably and both refer to an instance of a class. The term reference
> > > +refers to both C++ references and C++ pointers in their capacity to refer to an
> > > +object. Passing a reference means offering a way to a callee to obtain a
> > > +reference to an object that the caller has a valid reference to. Borrowing a
> > > +reference means using a reference passed by a caller without ownership transfer
> > > +based on the assumption that the caller guarantees the validate of the
> >
> > the validity
>
> Fixed.
>
> > > +reference for the duration of the operation that borrows the reference.
> >
> > maybe just "that borrows it."
>
> Fixed.
>
> > > +
> > > +#. Single Owner Objects
> > > +
> > > + * By default an object has a single owner at any time.
> >
> > "an object that has a"
>
> No, I really meant to say that by default an object has a single owner
> at any time, to explain that by default objects do not use shared
> ownership.
>
Ah right, I thougth this was the definition of what a "single owner
object" is...
> > > + * References to a single owner object can be borrowed by passing them from
> > > + the owner to the borrower, providing that
> >
> > Not sure this was what you wanted to express, but it seems to that
> > it would be more clear to state that "Ownership of a single owner
> > object can be borrowed, providing that"
>
> But then I can't use "reference" in the list below. How about
>
> "A single owner object can be borrowed by passing a reference from the
> owner to the borrower, providing that"
>
I still feel like it is actually the ownership that is
transferred/moved not the reference itself. This is fine anyway!
> > > +
> > > + * the owner guarantees the validity of the reference for the whole duration
> > > + of borrowing, and
> > > + * the borrower doesn't access the reference after the end of the borrowing.
> > > +
> > > + When borrowing from caller to callee for the duration of a function call,
> >
> > "When borrowing from caller to callee"
> > Can you borrow from callee to caller?
>
> Yes you can. CameraManager::instance() does exactly that, it returns a
> reference that is still owned by CameraManager can can be borrowed for
> the duration of the libcamera lifetime (which should be long enough :-)).
>
Right!
> > > + this implies that the callee shall not keep any stored reference after it
> > > + returns. These rules applies to the callee and all the functions it calls,
> >
> > rules -> apply
>
> Fixed.
>
> > > + directly or indirectly.
> >
> > * Ownership is borrowed by accessing the unique_ptr<> owned resources
> > in the caller and by declaring the callee function parameter as:
> > Your two points below here (const &) and pointer.
> > * const &
> > * pointers.
> >
Does this point not apply in your opinion?
> > > + * When the object doesn't need to be modified and may not be null, borrowed
> > > + references are passed as 'const &'.
> > > + * When the object may be modified or can be null, borrowed references are
> > > + passed as pointers. Unless otherwise specified, pointers passed to
> > > + functions are considered as borrowed references valid for the duration of
> > > + the function only.
> > > + * Single ownership is emphasized as much as possible by storing the unique
> > > + reference as a std::unique_ptr<>.
> >
> > Shouldn't this be (one of) the first points?
> >
> > > + * Ownership is transfered by passing the reference as a std::unique_ptr<>.
> >
> > s/transfered/transferred
>
> The spelling "transfered" exists:
> https://en.wiktionary.org/wiki/transfered. I'll still fix it :-)
>
Ah! my spell checker complained and I didn't look it up online
> > * Ownership is transfered by passing the reference as a std::unique_ptr<>.
> > or
> > * Ownership is transferred by declaring the callee functions
> > parameter as std::unique_ptr<>
> > (I would use the "by declaring the callee function parameter
> > as" in the description of owership borrowing too)
>
> How about "Ownership transfer is expressed by passing the reference as a
> std::unique_ptr<>." ? I'd like to avoid referring to function calls
> here, as that's not the only way to transfer ownership:
>
> struct Foo
> {
> std::unique_ptr<Bar> ptr;
> };
>
> Foo a;
> Foo b;
>
> b.ptr = std::move(a.ptr);
>
I see... I like your suggestion...
> (This will likely not appear as-is in our code of course.)
Hopefully not :)
>
> > Also, the use of std::unique_ptr<> as function argument implies the object
> > ownership gets moved to the callee so it won't be valid in the caller
> > afterwards (if not returned by the callee and re-assigned again in the
> > caller). Is this worth being described or is it clear enough in the
> > definition of what a unique_ptr<> is?
>
> I think it's clear from the std::unique_ptr<> definition and the concept
> of ownership transfer, but I can add the following sentence if you think
> it could be useful.
>
> "After ownership transfer the former owner has no valid reference to the
> object anymore and shall not access it without obtaining a valid
> reference."
I let you decide if it is worth or not
>
> > Furthermore, do you think moving ownership of a resource to a callee
> > function should be marked as a corner case, or indication of an issue
> > in the design, or do you see this happening often in practice?
>
> I think it can happen. See the setEventDispatcher() patch for instance.
> The function documentation states that the CameraManager takes ownership
> of the dispatcher, but with the patch the std::unique_ptr<> in the
> caller becomes null, ensuring that the caller will not erroneously try
> to delete the dispatcher later on.
>
and hopefully won't try to access it :)
> > > +
> > > +#. Shared Objects
> > > +
> > > + * Objects that may have multiple owners at a given time are called shared
> > > + objects. They are reference-counted and live as long as any references to
> > > + the object exist.
> > > + * Shared objects are created with std::make_shared<> or
> > > + std::allocate_shared<> an stored in an std::shared_ptr<>.
> > > + * Borrowed references to shared objects are passed with the same rules as for
> > > + single owner objects.
> >
> > Yes, but in this case 'borrowing' and 'transferring' of ownership have
> > different side effects on the object reference counting. I would
> > state that explicitly.
>
> Note that borrowing a reference to an object managed through a shared
> pointer is done by passing a reference to the object itself, not the
> shared pointer. I'll clarify this:
>
> * Borrowed references to shared objects are passed as references to the
> object itself, not to the std::shared_ptr<>, with the same rules as for
> single owner objects.
>
Why can't you borrow a reference as "shared_ptr<> &" ? That's
effectively borrowing a shared reference without increasing the
reference count, isn't it?
> > > + * Ownership is shared by creating and passing copies of any valid
> > > + std::shared_ptr<> reference. Ownership is released by destroying the
> > > + corresponding std::shared_ptr<>.
> > > +
> > > +.. attention:: Long term borrowing of single owner objects is allowed. Example
> > > + use cases are implementation of the singleton pattern (where the singleton
> > > + guarantees the validity of the reference forever), or returning references
> > > + to global objects whose lifetime matches the lifetime of the application. As
> > > + long term borrowing isn't marked through language constructs, it shall be
> > > + documented explicitly in details in the API.
> >
> > Thanks, this note is very useful imho.
> > Feel free to take in whatever you consider appropriate
>
> Thank you for your comments. Please let me know if you're fine with the
> changes I proposed above, and which ones should be included or dropped.
>
I'm fine, please go ahead merging this once you consider it ready.
Thanks
j
> > > +
> > >
> > > Tools
> > > -----
>
> --
> Regards,
>
> Laurent Pinchart
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190118/84997d85/attachment.sig>
More information about the libcamera-devel
mailing list