[libcamera-devel] [PATCH 1/4] Documentation: coding_style: Add object ownership rules
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat Jan 19 00:23:16 CET 2019
Hi Ricky,
On Sat, Jan 19, 2019 at 01:53:15AM +0800, Ricky Liang wrote:
> On Sat, Jan 19, 2019 at 1:41 AM Laurent Pinchart
> <laurent.pinchart at ideasonboard.com> wrote:
> > On Fri, Jan 18, 2019 at 06:00:44PM +0100, Jacopo Mondi wrote:
> >> On Fri, Jan 18, 2019 at 06:17:19PM +0200, Laurent Pinchart wrote:
> >>> 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!
> >
> > When transferring ownership, you're right, but when borrowing the
> > object, it's the object that you borrow, not its ownership. I'll ask you
> > to borrow your pen, not the ownership of your pen :-)
> >
> >>>>> +
> >>>>> + * 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?
> >
> > I missed it, sorry. I'll take this into account in v2.
> >
> >>>>> + * 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
> >
> > You spellchecker rightfully complained, as "transfered" is documented as
> > "Misspelling of transferred" :-)
> >
> >>>> * 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
> >
> > I'll add it.
> >
> >>>> 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?
> >
> > You can do that too, but it's more complex. Functions that borrow a
> > reference shouldn't be written assuming that the object uses shared_ptr,
> > unique_ptr or no smart pointer, they should take a reference or pointer
> > to the object directly.
>
> I'd vote against passing shared_ptr by reference (i.e. "shared_ptr<>
> &"). Semantically it's more clear if we always pass shared_ptr by
> value. The caller can decide whether it wants to duplicate the shared
> ownership (through copying the shared_ptr), or transferring its own
> share of the ownership to the callee (through std::move the
> shared_ptr).
I think that makes sense, I'll add a bullet point to mention this:
* When passed to a function, std::shared_ptr<> are always passed by value,
never by reference. The caller can decide whether to transfer its ownership
of the std::shared_ptr<> with std::move() or retain it. The callee shall
use std::move() if it needs to store the shared pointer.
> Some reference:
>
> https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#object-ownership-and-calling-conventions
Thank you for the link. I'll add it to the documentation.
> I understand we're following Google C++ style guide, but I find the
> Chromium C++ style guide useful as well. In the link scoped_refptr<T>
> is the Chromium's version of std::shared_ptr<T>.
We follow the Google C++ style guide as a base because we didn't feel
like writing a full style guide from scratch :-) We're certainly open to
adapting some of the Chromium-specific style rules when it makes sense,
such as the ones about object ownership.
> >>>>> + * 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.
> >>
> >>>>> +
> >>>>>
> >>>>> Tools
> >>>>> -----
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list