[libcamera-devel,1/4] Documentation: coding_style: Add object ownership rules

267 diff mbox series

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@ideasonboard.com>
---
 Documentation/coding-style.rst | 62 ++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

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...

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@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

> +reference for the duration of the operation that borrows the reference.

maybe just "that borrows it."

> +
> +#. Single Owner Objects
> +
> +   * By default an object has a single owner at any time.

"an object that has a"

> +   * 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"

> +
> +     * 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?

> +     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

> +     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.

> +   * 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

      * 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)

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?

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?

> +
> +#. 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.

> +   * 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

Thanks
  j

> +
>
>  Tools
>  -----
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Laurent,

Thanks for your work.

On 2019-01-18 01:59:13 +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@ideasonboard.com>

I think this is a really good starting point to document this complex 
topic.

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> ---
>  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
> +reference for the duration of the operation that borrows the reference.
> +
> +#. Single Owner Objects
> +
> +   * By default an object has a single owner at any time.
> +   * References to a single owner object can be borrowed by passing them from
> +     the owner to the borrower, providing that
> +
> +     * 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,
> +     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,
> +     directly or indirectly.
> +   * 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<>.
> +   * Ownership is transfered by passing the reference as a std::unique_ptr<>.
> +
> +#. 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.
> +   * 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.
> +
>  
>  Tools
>  -----
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

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 :-)

> 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@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.

> > +   * 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"

> > +
> > +     * 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 :-)).

> > +     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.
> 
> > +   * 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 :-)

>       * 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);

(This will likely not appear as-is in our code of course.)

> 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."

> 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.

> > +
> > +#. 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.

> > +   * 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.

> > +
> >
> >  Tools
> >  -----

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@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

Hi Jacopo,

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@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.

> >>> +   * 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
> >>> -----

Hi Laurent, Jacopo,

On Sat, Jan 19, 2019 at 1:41 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Jacopo,
>
> 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@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).

Some reference:

https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#object-ownership-and-calling-conventions

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>.

>
> > >>> +   * 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
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

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@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@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
> >>>>> -----