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

[Jacopo Mondi]

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

> +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 at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
-------------- 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/b6bc8a33/attachment.sig>