[{"id":406,"web_url":"https://patchwork.libcamera.org/comment/406/","msgid":"<20190118145608.qrypk6zilehgmk7m@uno.localdomain>","date":"2019-01-18T14:56:08","subject":"Re: [libcamera-devel] [PATCH 1/4] Documentation: coding_style: Add\n\tobject ownership rules","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n thanks for doing this!\n\nI don't look picky here, but this is an important piece of\ndocumentation, and I want to make sure I got it right first...\n\nOn Fri, Jan 18, 2019 at 01:59:13AM +0200, Laurent Pinchart wrote:\n> Object ownership is a complex topic that can lead to many issues, from\n> memory leak to crashes. Document the rules that libcamera enforces to\n> make object ownership tracking explicit.\n>\n> This is a first version of the rules and is expected to be expanded as\n> the library is developed.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> Documentation/coding-style.rst | 62 ++++++++++++++++++++++++++++++++++\n> 1 file changed, 62 insertions(+)\n>\n> diff --git a/Documentation/coding-style.rst b/Documentation/coding-style.rst\n> index f8d2fdfeda8e..f77325239bfa 100644\n> --- a/Documentation/coding-style.rst\n> +++ b/Documentation/coding-style.rst\n> @@ -81,6 +81,68 @@ C++-11-specific features:\n> overused.\n> * Variadic class and function templates\n>\n> +Object Ownership\n> +~~~~~~~~~~~~~~~~\n> +\n> +libcamera creates and destroys many objects at runtime, for both objects\n> +internal to the library and objects exposed to the user. To guarantee proper\n> +operation without use after free, double free or memory leaks, knowing who owns\n> +each object at any time is crucial. The project has enacted a set of rules to\n> +make object ownership tracking as explicit and fool-proof as possible.\n> +\n> +In the context of this section, the terms object and instance are used\n> +interchangeably and both refer to an instance of a class. The term reference\n> +refers to both C++ references and C++ pointers in their capacity to refer to an\n> +object. Passing a reference means offering a way to a callee to obtain a\n> +reference to an object that the caller has a valid reference to. Borrowing a\n> +reference means using a reference passed by a caller without ownership transfer\n> +based on the assumption that the caller guarantees the validate of the\n\nthe validity\n\n> +reference for the duration of the operation that borrows the reference.\n\nmaybe just \"that borrows it.\"\n\n> +\n> +#. Single Owner Objects\n> +\n> + * By default an object has a single owner at any time.\n\n\"an object that has a\"\n\n> + * References to a single owner object can be borrowed by passing them from\n> + the owner to the borrower, providing that\n\nNot sure this was what you wanted to express, but it seems to that\nit would be more clear to state that \"Ownership of a single owner\nobject can be borrowed, providing that\"\n\n> +\n> + * the owner guarantees the validity of the reference for the whole duration\n> + of borrowing, and\n> + * the borrower doesn't access the reference after the end of the borrowing.\n> +\n> + When borrowing from caller to callee for the duration of a function call,\n\n\"When borrowing from caller to callee\"\nCan you borrow from callee to caller?\n\n> + this implies that the callee shall not keep any stored reference after it\n> + returns. These rules applies to the callee and all the functions it calls,\n\nrules -> apply\n\n> + directly or indirectly.\n\n* Ownership is borrowed by accessing the unique_ptr<> owned resources\n in the caller and by declaring the callee function parameter as:\n Your two points below here (const &) and pointer.\n * const &\n * pointers.\n\n> + * When the object doesn't need to be modified and may not be null, borrowed\n> + references are passed as 'const &'.\n> + * When the object may be modified or can be null, borrowed references are\n> + passed as pointers. Unless otherwise specified, pointers passed to\n> + functions are considered as borrowed references valid for the duration of\n> + the function only.\n> + * Single ownership is emphasized as much as possible by storing the unique\n> + reference as a std::unique_ptr<>.\n\nShouldn't this be (one of) the first points?\n\n> + * Ownership is transfered by passing the reference as a std::unique_ptr<>.\n\ns/transfered/transferred\n\n * Ownership is transfered by passing the reference as a std::unique_ptr<>.\nor\n * Ownership is transferred by declaring the callee functions\n parameter as std::unique_ptr<>\n (I would use the \"by declaring the callee function parameter\n as\" in the description of owership borrowing too)\n\nAlso, the use of std::unique_ptr<> as function argument implies the object\nownership gets moved to the callee so it won't be valid in the caller\nafterwards (if not returned by the callee and re-assigned again in the\ncaller). Is this worth being described or is it clear enough in the\ndefinition of what a unique_ptr<> is?\n\nFurthermore, do you think moving ownership of a resource to a callee\nfunction should be marked as a corner case, or indication of an issue\nin the design, or do you see this happening often in practice?\n\n> +\n> +#. Shared Objects\n> +\n> + * Objects that may have multiple owners at a given time are called shared\n> + objects. They are reference-counted and live as long as any references to\n> + the object exist.\n> + * Shared objects are created with std::make_shared<> or\n> + std::allocate_shared<> an stored in an std::shared_ptr<>.\n> + * Borrowed references to shared objects are passed with the same rules as for\n> + single owner objects.\n\nYes, but in this case 'borrowing' and 'transferring' of ownership have\ndifferent side effects on the object reference counting. I would\nstate that explicitly.\n\n> + * Ownership is shared by creating and passing copies of any valid\n> + std::shared_ptr<> reference. Ownership is released by destroying the\n> + corresponding std::shared_ptr<>.\n> +\n> +.. attention:: Long term borrowing of single owner objects is allowed. Example\n> + use cases are implementation of the singleton pattern (where the singleton\n> + guarantees the validity of the reference forever), or returning references\n> + to global objects whose lifetime matches the lifetime of the application. As\n> + long term borrowing isn't marked through language constructs, it shall be\n> + documented explicitly in details in the API.\n\nThanks, this note is very useful imho.\nFeel free to take in whatever you consider appropriate\n\nThanks\n j\n\n> +\n>\n> Tools\n> -----\n> --\n> Regards,\n>\n> Laurent Pinchart\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 54A2F60B21\n\tfor ;\n\tFri, 18 Jan 2019 15:55:59 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id CBDD41C0010;\n\tFri, 18 Jan 2019 14:55:58 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Fri, 18 Jan 2019 15:56:08 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190118145608.qrypk6zilehgmk7m@uno.localdomain>","References":"<20190117235916.1906-1-laurent.pinchart@ideasonboard.com>\n\t<20190117235916.1906-2-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"mooodvwonnp5zn2y\"","Content-Disposition":"inline","In-Reply-To":"<20190117235916.1906-2-laurent.pinchart@ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 1/4] Documentation: coding_style: Add\n\tobject ownership rules","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 18 Jan 2019 14:56:03 -0000"}},{"id":412,"web_url":"https://patchwork.libcamera.org/comment/412/","msgid":"<20190118154626.GS6484@bigcity.dyn.berto.se>","date":"2019-01-18T15:46:26","subject":"Re: [libcamera-devel] [PATCH 1/4] Documentation: coding_style: Add\n\tobject ownership rules","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your work.\n\nOn 2019-01-18 01:59:13 +0200, Laurent Pinchart wrote:\n> Object ownership is a complex topic that can lead to many issues, from\n> memory leak to crashes. Document the rules that libcamera enforces to\n> make object ownership tracking explicit.\n> \n> This is a first version of the rules and is expected to be expanded as\n> the library is developed.\n> \n> Signed-off-by: Laurent Pinchart \n\nI think this is a really good starting point to document this complex \ntopic.\n\nReviewed-by: Niklas Söderlund \n\n> ---\n> Documentation/coding-style.rst | 62 ++++++++++++++++++++++++++++++++++\n> 1 file changed, 62 insertions(+)\n> \n> diff --git a/Documentation/coding-style.rst b/Documentation/coding-style.rst\n> index f8d2fdfeda8e..f77325239bfa 100644\n> --- a/Documentation/coding-style.rst\n> +++ b/Documentation/coding-style.rst\n> @@ -81,6 +81,68 @@ C++-11-specific features:\n> overused.\n> * Variadic class and function templates\n> \n> +Object Ownership\n> +~~~~~~~~~~~~~~~~\n> +\n> +libcamera creates and destroys many objects at runtime, for both objects\n> +internal to the library and objects exposed to the user. To guarantee proper\n> +operation without use after free, double free or memory leaks, knowing who owns\n> +each object at any time is crucial. The project has enacted a set of rules to\n> +make object ownership tracking as explicit and fool-proof as possible.\n> +\n> +In the context of this section, the terms object and instance are used\n> +interchangeably and both refer to an instance of a class. The term reference\n> +refers to both C++ references and C++ pointers in their capacity to refer to an\n> +object. Passing a reference means offering a way to a callee to obtain a\n> +reference to an object that the caller has a valid reference to. Borrowing a\n> +reference means using a reference passed by a caller without ownership transfer\n> +based on the assumption that the caller guarantees the validate of the\n> +reference for the duration of the operation that borrows the reference.\n> +\n> +#. Single Owner Objects\n> +\n> + * By default an object has a single owner at any time.\n> + * References to a single owner object can be borrowed by passing them from\n> + the owner to the borrower, providing that\n> +\n> + * the owner guarantees the validity of the reference for the whole duration\n> + of borrowing, and\n> + * the borrower doesn't access the reference after the end of the borrowing.\n> +\n> + When borrowing from caller to callee for the duration of a function call,\n> + this implies that the callee shall not keep any stored reference after it\n> + returns. These rules applies to the callee and all the functions it calls,\n> + directly or indirectly.\n> + * When the object doesn't need to be modified and may not be null, borrowed\n> + references are passed as 'const &'.\n> + * When the object may be modified or can be null, borrowed references are\n> + passed as pointers. Unless otherwise specified, pointers passed to\n> + functions are considered as borrowed references valid for the duration of\n> + the function only.\n> + * Single ownership is emphasized as much as possible by storing the unique\n> + reference as a std::unique_ptr<>.\n> + * Ownership is transfered by passing the reference as a std::unique_ptr<>.\n> +\n> +#. Shared Objects\n> +\n> + * Objects that may have multiple owners at a given time are called shared\n> + objects. They are reference-counted and live as long as any references to\n> + the object exist.\n> + * Shared objects are created with std::make_shared<> or\n> + std::allocate_shared<> an stored in an std::shared_ptr<>.\n> + * Borrowed references to shared objects are passed with the same rules as for\n> + single owner objects.\n> + * Ownership is shared by creating and passing copies of any valid\n> + std::shared_ptr<> reference. Ownership is released by destroying the\n> + corresponding std::shared_ptr<>.\n> +\n> +.. attention:: Long term borrowing of single owner objects is allowed. Example\n> + use cases are implementation of the singleton pattern (where the singleton\n> + guarantees the validity of the reference forever), or returning references\n> + to global objects whose lifetime matches the lifetime of the application. As\n> + long term borrowing isn't marked through language constructs, it shall be\n> + documented explicitly in details in the API.\n> +\n> \n> Tools\n> -----\n> -- \n> Regards,\n> \n> Laurent Pinchart\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from mail-lf1-x143.google.com (mail-lf1-x143.google.com\n\t[IPv6:2a00:1450:4864:20::143])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D8DE360B21\n\tfor ;\n\tFri, 18 Jan 2019 16:46:28 +0100 (CET)","by mail-lf1-x143.google.com with SMTP id c16so10808051lfj.8\n\tfor ;\n\tFri, 18 Jan 2019 07:46:28 -0800 (PST)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tw16-v6sm794330ljw.11.2019.01.18.07.46.27\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tFri, 18 Jan 2019 07:46:27 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=l1VEtxYzNJpy78tx+Ld73cDyyQrLv3aPQgbVUW6Idkc=;\n\tb=eRAmArQhYMqJvJ8hn3AKJHde+GrwFVUYaEFVtQm8r8DFLxGKDchO0dPQ66mi9KY+3C\n\tILin/HOwhP0ZmbgWwtwoPKT/3h9/xcWMh5vVbfctbEZvNOmrqbzLtfvse4Gwaw69rwgs\n\tb6a4Ovktp2yC6KGdN+kOMRMtcpvm4RdfE5ry791wooAjbTvtI16xDdMfT7xwAsVrQBGu\n\tbE2w/Fb4Rotk5nxYJ6BOkevv2P6dLUTZ4QPjQscU0XMYW2esenXR+ajSvkHZIFvkCoD6\n\t4j7H9VuYu8Fgs8qH4OcHd23H6w+GMVPaG4WytLhD38/i5/XsWU0d293LsU69mbQo93wc\n\tJBjQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=l1VEtxYzNJpy78tx+Ld73cDyyQrLv3aPQgbVUW6Idkc=;\n\tb=NGHR6mPze2mJdezgI/UpGJP3u4IoJAsaTonzqg9/gGpnk6+1znGAbfrl26MM7nvvZ8\n\tzgGHwurz/+jJ0MQGHISqfoJYZW9bb4AdcEhROe8HjcfSve/x9SENX2BrzHyxynuo2H8W\n\t22oFZ6j4sRg+AT8L/q5icInaYcrL5gJ8IxfD/YW94AG5K9ezfEl5E/aeCgEeA3KkzlFS\n\t2HvEWXJlK9Ep+MQNFrcHwqTZXvtijXHxsJLP+CAsBR1dMV0JENBogOvm38D7WDALNfJQ\n\tmiJ07b+nd8huAVglxqPBJqb3yB1oNT98e6PnBEtdLQfEkt8S+bWtArRWd1NOjEzdAjWs\n\tVp+g==","X-Gm-Message-State":"AJcUuken8XRjBWZAwmA1iM7NdXlAHR7kYQAYJLi+0FdMVyLIHZGZ3avF\n\tzBVCWfJ8nF4/ElyS0OljH4OQcv2c+6s=","X-Google-Smtp-Source":"ALg8bN6VJWFwILfRcJGZgQuXm/pTT+w3bQqMK1dcS1BMM4xsG8NILmQHDuDaH03/MNvcg0NMCKZp8Q==","X-Received":"by 2002:a19:920a:: with SMTP id\n\tu10mr8194782lfd.122.1547826387984; \n\tFri, 18 Jan 2019 07:46:27 -0800 (PST)","Date":"Fri, 18 Jan 2019 16:46:26 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190118154626.GS6484@bigcity.dyn.berto.se>","References":"<20190117235916.1906-1-laurent.pinchart@ideasonboard.com>\n\t<20190117235916.1906-2-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190117235916.1906-2-laurent.pinchart@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 1/4] Documentation: coding_style: Add\n\tobject ownership rules","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 18 Jan 2019 15:46:29 -0000"}},{"id":415,"web_url":"https://patchwork.libcamera.org/comment/415/","msgid":"<20190118161719.GI5275@pendragon.ideasonboard.com>","date":"2019-01-18T16:17:19","subject":"Re: [libcamera-devel] [PATCH 1/4] Documentation: coding_style: Add\n\tobject ownership rules","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Fri, Jan 18, 2019 at 03:56:08PM +0100, Jacopo Mondi wrote:\n> Hi Laurent,\n> thanks for doing this!\n> \n> I don't look picky here, but this is an important piece of\n> documentation, and I want to make sure I got it right first...\n\nThis is exactly the kind of patch for which I expect picky reviews :-)\n\n> On Fri, Jan 18, 2019 at 01:59:13AM +0200, Laurent Pinchart wrote:\n> > Object ownership is a complex topic that can lead to many issues, from\n> > memory leak to crashes. Document the rules that libcamera enforces to\n> > make object ownership tracking explicit.\n> >\n> > This is a first version of the rules and is expected to be expanded as\n> > the library is developed.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > Documentation/coding-style.rst | 62 ++++++++++++++++++++++++++++++++++\n> > 1 file changed, 62 insertions(+)\n> >\n> > diff --git a/Documentation/coding-style.rst b/Documentation/coding-style.rst\n> > index f8d2fdfeda8e..f77325239bfa 100644\n> > --- a/Documentation/coding-style.rst\n> > +++ b/Documentation/coding-style.rst\n> > @@ -81,6 +81,68 @@ C++-11-specific features:\n> > overused.\n> > * Variadic class and function templates\n> >\n> > +Object Ownership\n> > +~~~~~~~~~~~~~~~~\n> > +\n> > +libcamera creates and destroys many objects at runtime, for both objects\n> > +internal to the library and objects exposed to the user. To guarantee proper\n> > +operation without use after free, double free or memory leaks, knowing who owns\n> > +each object at any time is crucial. The project has enacted a set of rules to\n> > +make object ownership tracking as explicit and fool-proof as possible.\n> > +\n> > +In the context of this section, the terms object and instance are used\n> > +interchangeably and both refer to an instance of a class. The term reference\n> > +refers to both C++ references and C++ pointers in their capacity to refer to an\n> > +object. Passing a reference means offering a way to a callee to obtain a\n> > +reference to an object that the caller has a valid reference to. Borrowing a\n> > +reference means using a reference passed by a caller without ownership transfer\n> > +based on the assumption that the caller guarantees the validate of the\n> \n> the validity\n\nFixed.\n\n> > +reference for the duration of the operation that borrows the reference.\n> \n> maybe just \"that borrows it.\"\n\nFixed.\n\n> > +\n> > +#. Single Owner Objects\n> > +\n> > + * By default an object has a single owner at any time.\n> \n> \"an object that has a\"\n\nNo, I really meant to say that by default an object has a single owner\nat any time, to explain that by default objects do not use shared\nownership.\n\n> > + * References to a single owner object can be borrowed by passing them from\n> > + the owner to the borrower, providing that\n> \n> Not sure this was what you wanted to express, but it seems to that\n> it would be more clear to state that \"Ownership of a single owner\n> object can be borrowed, providing that\"\n\nBut then I can't use \"reference\" in the list below. How about\n\n\"A single owner object can be borrowed by passing a reference from the\nowner to the borrower, providing that\"\n\n> > +\n> > + * the owner guarantees the validity of the reference for the whole duration\n> > + of borrowing, and\n> > + * the borrower doesn't access the reference after the end of the borrowing.\n> > +\n> > + When borrowing from caller to callee for the duration of a function call,\n> \n> \"When borrowing from caller to callee\"\n> Can you borrow from callee to caller?\n\nYes you can. CameraManager::instance() does exactly that, it returns a\nreference that is still owned by CameraManager can can be borrowed for\nthe duration of the libcamera lifetime (which should be long enough :-)).\n\n> > + this implies that the callee shall not keep any stored reference after it\n> > + returns. These rules applies to the callee and all the functions it calls,\n> \n> rules -> apply\n\nFixed.\n\n> > + directly or indirectly.\n> \n> * Ownership is borrowed by accessing the unique_ptr<> owned resources\n> in the caller and by declaring the callee function parameter as:\n> Your two points below here (const &) and pointer.\n> * const &\n> * pointers.\n> \n> > + * When the object doesn't need to be modified and may not be null, borrowed\n> > + references are passed as 'const &'.\n> > + * When the object may be modified or can be null, borrowed references are\n> > + passed as pointers. Unless otherwise specified, pointers passed to\n> > + functions are considered as borrowed references valid for the duration of\n> > + the function only.\n> > + * Single ownership is emphasized as much as possible by storing the unique\n> > + reference as a std::unique_ptr<>.\n> \n> Shouldn't this be (one of) the first points?\n> \n> > + * Ownership is transfered by passing the reference as a std::unique_ptr<>.\n> \n> s/transfered/transferred\n\nThe spelling \"transfered\" exists:\nhttps://en.wiktionary.org/wiki/transfered. I'll still fix it :-)\n\n> * Ownership is transfered by passing the reference as a std::unique_ptr<>.\n> or\n> * Ownership is transferred by declaring the callee functions\n> parameter as std::unique_ptr<>\n> (I would use the \"by declaring the callee function parameter\n> as\" in the description of owership borrowing too)\n\nHow about \"Ownership transfer is expressed by passing the reference as a\nstd::unique_ptr<>.\" ? I'd like to avoid referring to function calls\nhere, as that's not the only way to transfer ownership:\n\nstruct Foo\n{\n\tstd::unique_ptr ptr;\n};\n\nFoo a;\nFoo b;\n\nb.ptr = std::move(a.ptr);\n\n(This will likely not appear as-is in our code of course.)\n\n> Also, the use of std::unique_ptr<> as function argument implies the object\n> ownership gets moved to the callee so it won't be valid in the caller\n> afterwards (if not returned by the callee and re-assigned again in the\n> caller). Is this worth being described or is it clear enough in the\n> definition of what a unique_ptr<> is?\n\nI think it's clear from the std::unique_ptr<> definition and the concept\nof ownership transfer, but I can add the following sentence if you think\nit could be useful.\n\n\"After ownership transfer the former owner has no valid reference to the\nobject anymore and shall not access it without obtaining a valid\nreference.\"\n\n> Furthermore, do you think moving ownership of a resource to a callee\n> function should be marked as a corner case, or indication of an issue\n> in the design, or do you see this happening often in practice?\n\nI think it can happen. See the setEventDispatcher() patch for instance.\nThe function documentation states that the CameraManager takes ownership\nof the dispatcher, but with the patch the std::unique_ptr<> in the\ncaller becomes null, ensuring that the caller will not erroneously try\nto delete the dispatcher later on.\n\n> > +\n> > +#. Shared Objects\n> > +\n> > + * Objects that may have multiple owners at a given time are called shared\n> > + objects. They are reference-counted and live as long as any references to\n> > + the object exist.\n> > + * Shared objects are created with std::make_shared<> or\n> > + std::allocate_shared<> an stored in an std::shared_ptr<>.\n> > + * Borrowed references to shared objects are passed with the same rules as for\n> > + single owner objects.\n> \n> Yes, but in this case 'borrowing' and 'transferring' of ownership have\n> different side effects on the object reference counting. I would\n> state that explicitly.\n\nNote that borrowing a reference to an object managed through a shared\npointer is done by passing a reference to the object itself, not the\nshared pointer. I'll clarify this:\n\n * Borrowed references to shared objects are passed as references to the\n object itself, not to the std::shared_ptr<>, with the same rules as for\n single owner objects.\n\n> > + * Ownership is shared by creating and passing copies of any valid\n> > + std::shared_ptr<> reference. Ownership is released by destroying the\n> > + corresponding std::shared_ptr<>.\n> > +\n> > +.. attention:: Long term borrowing of single owner objects is allowed. Example\n> > + use cases are implementation of the singleton pattern (where the singleton\n> > + guarantees the validity of the reference forever), or returning references\n> > + to global objects whose lifetime matches the lifetime of the application. As\n> > + long term borrowing isn't marked through language constructs, it shall be\n> > + documented explicitly in details in the API.\n> \n> Thanks, this note is very useful imho.\n> Feel free to take in whatever you consider appropriate\n\nThank you for your comments. Please let me know if you're fine with the\nchanges I proposed above, and which ones should be included or dropped.\n\n> > +\n> >\n> > Tools\n> > -----","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 09D3660B21\n\tfor ;\n\tFri, 18 Jan 2019 17:17:20 +0100 (CET)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6739753E;\n\tFri, 18 Jan 2019 17:17:19 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1547828239;\n\tbh=0jrFlr1XuHAJgOa5GC8Q/Lt+bpBKB/TZrmSk+dW90dw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lW0P8OkqRLj2HHKL1J1Hzk0j9HcJz5KDTp3w0ZAs5uDijYP64FgrCfxAlU7g3JZiK\n\t8PkcT8k4a626VlqDlahkOM0QKth23xLAwyyWLj/LjR7+ut6SPusWyNSoc+UhUg5bnx\n\tvLXfURG5jFTYlI9pDJr+wxi3zxQLE7B2i8fT5V5U=","Date":"Fri, 18 Jan 2019 18:17:19 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190118161719.GI5275@pendragon.ideasonboard.com>","References":"<20190117235916.1906-1-laurent.pinchart@ideasonboard.com>\n\t<20190117235916.1906-2-laurent.pinchart@ideasonboard.com>\n\t<20190118145608.qrypk6zilehgmk7m@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190118145608.qrypk6zilehgmk7m@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 1/4] Documentation: coding_style: Add\n\tobject ownership rules","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 18 Jan 2019 16:17:20 -0000"}},{"id":420,"web_url":"https://patchwork.libcamera.org/comment/420/","msgid":"<20190118170044.ae4e77kvn6sc5yog@uno.localdomain>","date":"2019-01-18T17:00:44","subject":"Re: [libcamera-devel] [PATCH 1/4] Documentation: coding_style: Add\n\tobject ownership rules","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Fri, Jan 18, 2019 at 06:17:19PM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Fri, Jan 18, 2019 at 03:56:08PM +0100, Jacopo Mondi wrote:\n> > Hi Laurent,\n> > thanks for doing this!\n> >\n> > I don't look picky here, but this is an important piece of\n> > documentation, and I want to make sure I got it right first...\n>\n> This is exactly the kind of patch for which I expect picky reviews :-)\n>\n\nHappy to be of any help then :)\n\n> > On Fri, Jan 18, 2019 at 01:59:13AM +0200, Laurent Pinchart wrote:\n> > > Object ownership is a complex topic that can lead to many issues, from\n> > > memory leak to crashes. Document the rules that libcamera enforces to\n> > > make object ownership tracking explicit.\n> > >\n> > > This is a first version of the rules and is expected to be expanded as\n> > > the library is developed.\n> > >\n> > > Signed-off-by: Laurent Pinchart \n> > > ---\n> > > Documentation/coding-style.rst | 62 ++++++++++++++++++++++++++++++++++\n> > > 1 file changed, 62 insertions(+)\n> > >\n> > > diff --git a/Documentation/coding-style.rst b/Documentation/coding-style.rst\n> > > index f8d2fdfeda8e..f77325239bfa 100644\n> > > --- a/Documentation/coding-style.rst\n> > > +++ b/Documentation/coding-style.rst\n> > > @@ -81,6 +81,68 @@ C++-11-specific features:\n> > > overused.\n> > > * Variadic class and function templates\n> > >\n> > > +Object Ownership\n> > > +~~~~~~~~~~~~~~~~\n> > > +\n> > > +libcamera creates and destroys many objects at runtime, for both objects\n> > > +internal to the library and objects exposed to the user. To guarantee proper\n> > > +operation without use after free, double free or memory leaks, knowing who owns\n> > > +each object at any time is crucial. The project has enacted a set of rules to\n> > > +make object ownership tracking as explicit and fool-proof as possible.\n> > > +\n> > > +In the context of this section, the terms object and instance are used\n> > > +interchangeably and both refer to an instance of a class. The term reference\n> > > +refers to both C++ references and C++ pointers in their capacity to refer to an\n> > > +object. Passing a reference means offering a way to a callee to obtain a\n> > > +reference to an object that the caller has a valid reference to. Borrowing a\n> > > +reference means using a reference passed by a caller without ownership transfer\n> > > +based on the assumption that the caller guarantees the validate of the\n> >\n> > the validity\n>\n> Fixed.\n>\n> > > +reference for the duration of the operation that borrows the reference.\n> >\n> > maybe just \"that borrows it.\"\n>\n> Fixed.\n>\n> > > +\n> > > +#. Single Owner Objects\n> > > +\n> > > + * By default an object has a single owner at any time.\n> >\n> > \"an object that has a\"\n>\n> No, I really meant to say that by default an object has a single owner\n> at any time, to explain that by default objects do not use shared\n> ownership.\n>\n\nAh right, I thougth this was the definition of what a \"single owner\nobject\" is...\n\n\n> > > + * References to a single owner object can be borrowed by passing them from\n> > > + the owner to the borrower, providing that\n> >\n> > Not sure this was what you wanted to express, but it seems to that\n> > it would be more clear to state that \"Ownership of a single owner\n> > object can be borrowed, providing that\"\n>\n> But then I can't use \"reference\" in the list below. How about\n>\n> \"A single owner object can be borrowed by passing a reference from the\n> owner to the borrower, providing that\"\n>\n\nI still feel like it is actually the ownership that is\ntransferred/moved not the reference itself. This is fine anyway!\n\n> > > +\n> > > + * the owner guarantees the validity of the reference for the whole duration\n> > > + of borrowing, and\n> > > + * the borrower doesn't access the reference after the end of the borrowing.\n> > > +\n> > > + When borrowing from caller to callee for the duration of a function call,\n> >\n> > \"When borrowing from caller to callee\"\n> > Can you borrow from callee to caller?\n>\n> Yes you can. CameraManager::instance() does exactly that, it returns a\n> reference that is still owned by CameraManager can can be borrowed for\n> the duration of the libcamera lifetime (which should be long enough :-)).\n>\n\nRight!\n\n> > > + this implies that the callee shall not keep any stored reference after it\n> > > + returns. These rules applies to the callee and all the functions it calls,\n> >\n> > rules -> apply\n>\n> Fixed.\n>\n> > > + directly or indirectly.\n> >\n> > * Ownership is borrowed by accessing the unique_ptr<> owned resources\n> > in the caller and by declaring the callee function parameter as:\n> > Your two points below here (const &) and pointer.\n> > * const &\n> > * pointers.\n> >\n\nDoes this point not apply in your opinion?\n\n> > > + * When the object doesn't need to be modified and may not be null, borrowed\n> > > + references are passed as 'const &'.\n> > > + * When the object may be modified or can be null, borrowed references are\n> > > + passed as pointers. Unless otherwise specified, pointers passed to\n> > > + functions are considered as borrowed references valid for the duration of\n> > > + the function only.\n> > > + * Single ownership is emphasized as much as possible by storing the unique\n> > > + reference as a std::unique_ptr<>.\n> >\n> > Shouldn't this be (one of) the first points?\n> >\n> > > + * Ownership is transfered by passing the reference as a std::unique_ptr<>.\n> >\n> > s/transfered/transferred\n>\n> The spelling \"transfered\" exists:\n> https://en.wiktionary.org/wiki/transfered. I'll still fix it :-)\n>\n\nAh! my spell checker complained and I didn't look it up online\n\n> > * Ownership is transfered by passing the reference as a std::unique_ptr<>.\n> > or\n> > * Ownership is transferred by declaring the callee functions\n> > parameter as std::unique_ptr<>\n> > (I would use the \"by declaring the callee function parameter\n> > as\" in the description of owership borrowing too)\n>\n> How about \"Ownership transfer is expressed by passing the reference as a\n> std::unique_ptr<>.\" ? I'd like to avoid referring to function calls\n> here, as that's not the only way to transfer ownership:\n>\n> struct Foo\n> {\n> \tstd::unique_ptr ptr;\n> };\n>\n> Foo a;\n> Foo b;\n>\n> b.ptr = std::move(a.ptr);\n>\n\nI see... I like your suggestion...\n\n> (This will likely not appear as-is in our code of course.)\n\nHopefully not :)\n\n>\n> > Also, the use of std::unique_ptr<> as function argument implies the object\n> > ownership gets moved to the callee so it won't be valid in the caller\n> > afterwards (if not returned by the callee and re-assigned again in the\n> > caller). Is this worth being described or is it clear enough in the\n> > definition of what a unique_ptr<> is?\n>\n> I think it's clear from the std::unique_ptr<> definition and the concept\n> of ownership transfer, but I can add the following sentence if you think\n> it could be useful.\n>\n> \"After ownership transfer the former owner has no valid reference to the\n> object anymore and shall not access it without obtaining a valid\n> reference.\"\n\nI let you decide if it is worth or not\n\n>\n> > Furthermore, do you think moving ownership of a resource to a callee\n> > function should be marked as a corner case, or indication of an issue\n> > in the design, or do you see this happening often in practice?\n>\n> I think it can happen. See the setEventDispatcher() patch for instance.\n> The function documentation states that the CameraManager takes ownership\n> of the dispatcher, but with the patch the std::unique_ptr<> in the\n> caller becomes null, ensuring that the caller will not erroneously try\n> to delete the dispatcher later on.\n>\n\nand hopefully won't try to access it :)\n\n> > > +\n> > > +#. Shared Objects\n> > > +\n> > > + * Objects that may have multiple owners at a given time are called shared\n> > > + objects. They are reference-counted and live as long as any references to\n> > > + the object exist.\n> > > + * Shared objects are created with std::make_shared<> or\n> > > + std::allocate_shared<> an stored in an std::shared_ptr<>.\n> > > + * Borrowed references to shared objects are passed with the same rules as for\n> > > + single owner objects.\n> >\n> > Yes, but in this case 'borrowing' and 'transferring' of ownership have\n> > different side effects on the object reference counting. I would\n> > state that explicitly.\n>\n> Note that borrowing a reference to an object managed through a shared\n> pointer is done by passing a reference to the object itself, not the\n> shared pointer. I'll clarify this:\n>\n> * Borrowed references to shared objects are passed as references to the\n> object itself, not to the std::shared_ptr<>, with the same rules as for\n> single owner objects.\n>\n\nWhy can't you borrow a reference as \"shared_ptr<> &\" ? That's\neffectively borrowing a shared reference without increasing the\nreference count, isn't it?\n\n\n> > > + * Ownership is shared by creating and passing copies of any valid\n> > > + std::shared_ptr<> reference. Ownership is released by destroying the\n> > > + corresponding std::shared_ptr<>.\n> > > +\n> > > +.. attention:: Long term borrowing of single owner objects is allowed. Example\n> > > + use cases are implementation of the singleton pattern (where the singleton\n> > > + guarantees the validity of the reference forever), or returning references\n> > > + to global objects whose lifetime matches the lifetime of the application. As\n> > > + long term borrowing isn't marked through language constructs, it shall be\n> > > + documented explicitly in details in the API.\n> >\n> > Thanks, this note is very useful imho.\n> > Feel free to take in whatever you consider appropriate\n>\n> Thank you for your comments. Please let me know if you're fine with the\n> changes I proposed above, and which ones should be included or dropped.\n>\n\nI'm fine, please go ahead merging this once you consider it ready.\n\nThanks\n j\n\n> > > +\n> > >\n> > > Tools\n> > > -----\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5C7C360B21\n\tfor ;\n\tFri, 18 Jan 2019 18:00:37 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id 887931C0014;\n\tFri, 18 Jan 2019 17:00:35 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Fri, 18 Jan 2019 18:00:44 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190118170044.ae4e77kvn6sc5yog@uno.localdomain>","References":"<20190117235916.1906-1-laurent.pinchart@ideasonboard.com>\n\t<20190117235916.1906-2-laurent.pinchart@ideasonboard.com>\n\t<20190118145608.qrypk6zilehgmk7m@uno.localdomain>\n\t<20190118161719.GI5275@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"6iwbj3zf4lbijlmb\"","Content-Disposition":"inline","In-Reply-To":"<20190118161719.GI5275@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 1/4] Documentation: coding_style: Add\n\tobject ownership rules","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 18 Jan 2019 17:00:37 -0000"}},{"id":423,"web_url":"https://patchwork.libcamera.org/comment/423/","msgid":"<20190118174131.GA1799@pendragon.ideasonboard.com>","date":"2019-01-18T17:41:31","subject":"Re: [libcamera-devel] [PATCH 1/4] Documentation: coding_style: Add\n\tobject ownership rules","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Fri, Jan 18, 2019 at 06:00:44PM +0100, Jacopo Mondi wrote:\n> On Fri, Jan 18, 2019 at 06:17:19PM +0200, Laurent Pinchart wrote:\n> > On Fri, Jan 18, 2019 at 03:56:08PM +0100, Jacopo Mondi wrote:\n> >> Hi Laurent,\n> >> thanks for doing this!\n> >> \n> >> I don't look picky here, but this is an important piece of\n> >> documentation, and I want to make sure I got it right first...\n> > \n> > This is exactly the kind of patch for which I expect picky reviews :-)\n> > \n> \n> Happy to be of any help then :)\n> \n> >> On Fri, Jan 18, 2019 at 01:59:13AM +0200, Laurent Pinchart wrote:\n> >>> Object ownership is a complex topic that can lead to many issues, from\n> >>> memory leak to crashes. Document the rules that libcamera enforces to\n> >>> make object ownership tracking explicit.\n> >>> \n> >>> This is a first version of the rules and is expected to be expanded as\n> >>> the library is developed.\n> >>> \n> >>> Signed-off-by: Laurent Pinchart \n> >>> ---\n> >>> Documentation/coding-style.rst | 62 ++++++++++++++++++++++++++++++++++\n> >>> 1 file changed, 62 insertions(+)\n> >>> \n> >>> diff --git a/Documentation/coding-style.rst b/Documentation/coding-style.rst\n> >>> index f8d2fdfeda8e..f77325239bfa 100644\n> >>> --- a/Documentation/coding-style.rst\n> >>> +++ b/Documentation/coding-style.rst\n> >>> @@ -81,6 +81,68 @@ C++-11-specific features:\n> >>> overused.\n> >>> * Variadic class and function templates\n> >>> \n> >>> +Object Ownership\n> >>> +~~~~~~~~~~~~~~~~\n> >>> +\n> >>> +libcamera creates and destroys many objects at runtime, for both objects\n> >>> +internal to the library and objects exposed to the user. To guarantee proper\n> >>> +operation without use after free, double free or memory leaks, knowing who owns\n> >>> +each object at any time is crucial. The project has enacted a set of rules to\n> >>> +make object ownership tracking as explicit and fool-proof as possible.\n> >>> +\n> >>> +In the context of this section, the terms object and instance are used\n> >>> +interchangeably and both refer to an instance of a class. The term reference\n> >>> +refers to both C++ references and C++ pointers in their capacity to refer to an\n> >>> +object. Passing a reference means offering a way to a callee to obtain a\n> >>> +reference to an object that the caller has a valid reference to. Borrowing a\n> >>> +reference means using a reference passed by a caller without ownership transfer\n> >>> +based on the assumption that the caller guarantees the validate of the\n> >> \n> >> the validity\n> > \n> > Fixed.\n> > \n> >>> +reference for the duration of the operation that borrows the reference.\n> >> \n> >> maybe just \"that borrows it.\"\n> > \n> > Fixed.\n> > \n> >>> +\n> >>> +#. Single Owner Objects\n> >>> +\n> >>> + * By default an object has a single owner at any time.\n> >> \n> >> \"an object that has a\"\n> > \n> > No, I really meant to say that by default an object has a single owner\n> > at any time, to explain that by default objects do not use shared\n> > ownership.\n> > \n> \n> Ah right, I thougth this was the definition of what a \"single owner\n> object\" is...\n> \n> \n> >>> + * References to a single owner object can be borrowed by passing them from\n> >>> + the owner to the borrower, providing that\n> >> \n> >> Not sure this was what you wanted to express, but it seems to that\n> >> it would be more clear to state that \"Ownership of a single owner\n> >> object can be borrowed, providing that\"\n> > \n> > But then I can't use \"reference\" in the list below. How about\n> > \n> > \"A single owner object can be borrowed by passing a reference from the\n> > owner to the borrower, providing that\"\n> > \n> I still feel like it is actually the ownership that is\n> transferred/moved not the reference itself. This is fine anyway!\n\nWhen transferring ownership, you're right, but when borrowing the\nobject, it's the object that you borrow, not its ownership. I'll ask you\nto borrow your pen, not the ownership of your pen :-)\n\n> >>> +\n> >>> + * the owner guarantees the validity of the reference for the whole duration\n> >>> + of borrowing, and\n> >>> + * the borrower doesn't access the reference after the end of the borrowing.\n> >>> +\n> >>> + When borrowing from caller to callee for the duration of a function call,\n> >> \n> >> \"When borrowing from caller to callee\"\n> >> Can you borrow from callee to caller?\n> > \n> > Yes you can. CameraManager::instance() does exactly that, it returns a\n> > reference that is still owned by CameraManager can can be borrowed for\n> > the duration of the libcamera lifetime (which should be long enough :-)).\n> > \n> \n> Right!\n> \n> >>> + this implies that the callee shall not keep any stored reference after it\n> >>> + returns. These rules applies to the callee and all the functions it calls,\n> >> \n> >> rules -> apply\n> > \n> > Fixed.\n> > \n> >>> + directly or indirectly.\n> >> \n> >> * Ownership is borrowed by accessing the unique_ptr<> owned resources\n> >> in the caller and by declaring the callee function parameter as:\n> >> Your two points below here (const &) and pointer.\n> >> * const &\n> >> * pointers.\n> \n> Does this point not apply in your opinion?\n\nI missed it, sorry. I'll take this into account in v2.\n\n> >>> + * When the object doesn't need to be modified and may not be null, borrowed\n> >>> + references are passed as 'const &'.\n> >>> + * When the object may be modified or can be null, borrowed references are\n> >>> + passed as pointers. Unless otherwise specified, pointers passed to\n> >>> + functions are considered as borrowed references valid for the duration of\n> >>> + the function only.\n> >>> + * Single ownership is emphasized as much as possible by storing the unique\n> >>> + reference as a std::unique_ptr<>.\n> >> \n> >> Shouldn't this be (one of) the first points?\n> >> \n> >>> + * Ownership is transfered by passing the reference as a std::unique_ptr<>.\n> >> \n> >> s/transfered/transferred\n> > \n> > The spelling \"transfered\" exists:\n> > https://en.wiktionary.org/wiki/transfered. I'll still fix it :-)\n> > \n> \n> Ah! my spell checker complained and I didn't look it up online\n\nYou spellchecker rightfully complained, as \"transfered\" is documented as\n\"Misspelling of transferred\" :-)\n\n> >> * Ownership is transfered by passing the reference as a std::unique_ptr<>.\n> >> or\n> >> * Ownership is transferred by declaring the callee functions\n> >> parameter as std::unique_ptr<>\n> >> (I would use the \"by declaring the callee function parameter\n> >> as\" in the description of owership borrowing too)\n> > \n> > How about \"Ownership transfer is expressed by passing the reference as a\n> > std::unique_ptr<>.\" ? I'd like to avoid referring to function calls\n> > here, as that's not the only way to transfer ownership:\n> > \n> > struct Foo\n> > {\n> > \tstd::unique_ptr ptr;\n> > };\n> > \n> > Foo a;\n> > Foo b;\n> > \n> > b.ptr = std::move(a.ptr);\n> > \n> \n> I see... I like your suggestion...\n> \n> > (This will likely not appear as-is in our code of course.)\n> \n> Hopefully not :)\n> \n> > \n> >> Also, the use of std::unique_ptr<> as function argument implies the object\n> >> ownership gets moved to the callee so it won't be valid in the caller\n> >> afterwards (if not returned by the callee and re-assigned again in the\n> >> caller). Is this worth being described or is it clear enough in the\n> >> definition of what a unique_ptr<> is?\n> > \n> > I think it's clear from the std::unique_ptr<> definition and the concept\n> > of ownership transfer, but I can add the following sentence if you think\n> > it could be useful.\n> > \n> > \"After ownership transfer the former owner has no valid reference to the\n> > object anymore and shall not access it without obtaining a valid\n> > reference.\"\n> \n> I let you decide if it is worth or not\n\nI'll add it.\n\n> >> Furthermore, do you think moving ownership of a resource to a callee\n> >> function should be marked as a corner case, or indication of an issue\n> >> in the design, or do you see this happening often in practice?\n> > \n> > I think it can happen. See the setEventDispatcher() patch for instance.\n> > The function documentation states that the CameraManager takes ownership\n> > of the dispatcher, but with the patch the std::unique_ptr<> in the\n> > caller becomes null, ensuring that the caller will not erroneously try\n> > to delete the dispatcher later on.\n> \n> and hopefully won't try to access it :)\n> \n> >>> +\n> >>> +#. Shared Objects\n> >>> +\n> >>> + * Objects that may have multiple owners at a given time are called shared\n> >>> + objects. They are reference-counted and live as long as any references to\n> >>> + the object exist.\n> >>> + * Shared objects are created with std::make_shared<> or\n> >>> + std::allocate_shared<> an stored in an std::shared_ptr<>.\n> >>> + * Borrowed references to shared objects are passed with the same rules as for\n> >>> + single owner objects.\n> >> \n> >> Yes, but in this case 'borrowing' and 'transferring' of ownership have\n> >> different side effects on the object reference counting. I would\n> >> state that explicitly.\n> > \n> > Note that borrowing a reference to an object managed through a shared\n> > pointer is done by passing a reference to the object itself, not the\n> > shared pointer. I'll clarify this:\n> > \n> > * Borrowed references to shared objects are passed as references to the\n> > object itself, not to the std::shared_ptr<>, with the same rules as for\n> > single owner objects.\n> \n> Why can't you borrow a reference as \"shared_ptr<> &\" ? That's\n> effectively borrowing a shared reference without increasing the\n> reference count, isn't it?\n\nYou can do that too, but it's more complex. Functions that borrow a\nreference shouldn't be written assuming that the object uses shared_ptr,\nunique_ptr or no smart pointer, they should take a reference or pointer\nto the object directly.\n\n> >>> + * Ownership is shared by creating and passing copies of any valid\n> >>> + std::shared_ptr<> reference. Ownership is released by destroying the\n> >>> + corresponding std::shared_ptr<>.\n> >>> +\n> >>> +.. attention:: Long term borrowing of single owner objects is allowed. Example\n> >>> + use cases are implementation of the singleton pattern (where the singleton\n> >>> + guarantees the validity of the reference forever), or returning references\n> >>> + to global objects whose lifetime matches the lifetime of the application. As\n> >>> + long term borrowing isn't marked through language constructs, it shall be\n> >>> + documented explicitly in details in the API.\n> >> \n> >> Thanks, this note is very useful imho.\n> >> Feel free to take in whatever you consider appropriate\n> > \n> > Thank you for your comments. Please let me know if you're fine with the\n> > changes I proposed above, and which ones should be included or dropped.\n> > \n> \n> I'm fine, please go ahead merging this once you consider it ready.\n> \n> >>> +\n> >>> \n> >>> Tools\n> >>> -----","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CB37F60B21\n\tfor ;\n\tFri, 18 Jan 2019 18:41:31 +0100 (CET)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 47C6F53E;\n\tFri, 18 Jan 2019 18:41:31 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1547833291;\n\tbh=zqhRwjprkobwBK9YafFmBk9sjkzXDcAT9XO/qDszqYQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=nkDf5PtvFRdsFxAXgVFvkNqQ4fMs3JDq9Bo6fGD2QY3V7pajKNwhnkZ5fHDHpVkBU\n\tfXM3HS6rhAzq0dFQmF9V7Q9H/bgF/WRdyTcNqW8wVkZPvimALh+i61P/x1HslJJAsm\n\tZwkk0s/z5g7vL2J6eqW6HZxmWkdKLTbrNeSjVh00=","Date":"Fri, 18 Jan 2019 19:41:31 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190118174131.GA1799@pendragon.ideasonboard.com>","References":"<20190117235916.1906-1-laurent.pinchart@ideasonboard.com>\n\t<20190117235916.1906-2-laurent.pinchart@ideasonboard.com>\n\t<20190118145608.qrypk6zilehgmk7m@uno.localdomain>\n\t<20190118161719.GI5275@pendragon.ideasonboard.com>\n\t<20190118170044.ae4e77kvn6sc5yog@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190118170044.ae4e77kvn6sc5yog@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 1/4] Documentation: coding_style: Add\n\tobject ownership rules","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 18 Jan 2019 17:41:32 -0000"}},{"id":424,"web_url":"https://patchwork.libcamera.org/comment/424/","msgid":"","date":"2019-01-18T17:53:15","subject":"Re: [libcamera-devel] [PATCH 1/4] Documentation: coding_style: Add\n\tobject ownership rules","submitter":{"id":6,"url":"https://patchwork.libcamera.org/api/people/6/","name":"Ricky Liang","email":"jcliang@chromium.org"},"content":"Hi Laurent, Jacopo,\n\nOn Sat, Jan 19, 2019 at 1:41 AM Laurent Pinchart\n wrote:\n>\n> Hi Jacopo,\n>\n> On Fri, Jan 18, 2019 at 06:00:44PM +0100, Jacopo Mondi wrote:\n> > On Fri, Jan 18, 2019 at 06:17:19PM +0200, Laurent Pinchart wrote:\n> > > On Fri, Jan 18, 2019 at 03:56:08PM +0100, Jacopo Mondi wrote:\n> > >> Hi Laurent,\n> > >> thanks for doing this!\n> > >>\n> > >> I don't look picky here, but this is an important piece of\n> > >> documentation, and I want to make sure I got it right first...\n> > >\n> > > This is exactly the kind of patch for which I expect picky reviews :-)\n> > >\n> >\n> > Happy to be of any help then :)\n> >\n> > >> On Fri, Jan 18, 2019 at 01:59:13AM +0200, Laurent Pinchart wrote:\n> > >>> Object ownership is a complex topic that can lead to many issues, from\n> > >>> memory leak to crashes. Document the rules that libcamera enforces to\n> > >>> make object ownership tracking explicit.\n> > >>>\n> > >>> This is a first version of the rules and is expected to be expanded as\n> > >>> the library is developed.\n> > >>>\n> > >>> Signed-off-by: Laurent Pinchart \n> > >>> ---\n> > >>> Documentation/coding-style.rst | 62 ++++++++++++++++++++++++++++++++++\n> > >>> 1 file changed, 62 insertions(+)\n> > >>>\n> > >>> diff --git a/Documentation/coding-style.rst b/Documentation/coding-style.rst\n> > >>> index f8d2fdfeda8e..f77325239bfa 100644\n> > >>> --- a/Documentation/coding-style.rst\n> > >>> +++ b/Documentation/coding-style.rst\n> > >>> @@ -81,6 +81,68 @@ C++-11-specific features:\n> > >>> overused.\n> > >>> * Variadic class and function templates\n> > >>>\n> > >>> +Object Ownership\n> > >>> +~~~~~~~~~~~~~~~~\n> > >>> +\n> > >>> +libcamera creates and destroys many objects at runtime, for both objects\n> > >>> +internal to the library and objects exposed to the user. To guarantee proper\n> > >>> +operation without use after free, double free or memory leaks, knowing who owns\n> > >>> +each object at any time is crucial. The project has enacted a set of rules to\n> > >>> +make object ownership tracking as explicit and fool-proof as possible.\n> > >>> +\n> > >>> +In the context of this section, the terms object and instance are used\n> > >>> +interchangeably and both refer to an instance of a class. The term reference\n> > >>> +refers to both C++ references and C++ pointers in their capacity to refer to an\n> > >>> +object. Passing a reference means offering a way to a callee to obtain a\n> > >>> +reference to an object that the caller has a valid reference to. Borrowing a\n> > >>> +reference means using a reference passed by a caller without ownership transfer\n> > >>> +based on the assumption that the caller guarantees the validate of the\n> > >>\n> > >> the validity\n> > >\n> > > Fixed.\n> > >\n> > >>> +reference for the duration of the operation that borrows the reference.\n> > >>\n> > >> maybe just \"that borrows it.\"\n> > >\n> > > Fixed.\n> > >\n> > >>> +\n> > >>> +#. Single Owner Objects\n> > >>> +\n> > >>> + * By default an object has a single owner at any time.\n> > >>\n> > >> \"an object that has a\"\n> > >\n> > > No, I really meant to say that by default an object has a single owner\n> > > at any time, to explain that by default objects do not use shared\n> > > ownership.\n> > >\n> >\n> > Ah right, I thougth this was the definition of what a \"single owner\n> > object\" is...\n> >\n> >\n> > >>> + * References to a single owner object can be borrowed by passing them from\n> > >>> + the owner to the borrower, providing that\n> > >>\n> > >> Not sure this was what you wanted to express, but it seems to that\n> > >> it would be more clear to state that \"Ownership of a single owner\n> > >> object can be borrowed, providing that\"\n> > >\n> > > But then I can't use \"reference\" in the list below. How about\n> > >\n> > > \"A single owner object can be borrowed by passing a reference from the\n> > > owner to the borrower, providing that\"\n> > >\n> > I still feel like it is actually the ownership that is\n> > transferred/moved not the reference itself. This is fine anyway!\n>\n> When transferring ownership, you're right, but when borrowing the\n> object, it's the object that you borrow, not its ownership. I'll ask you\n> to borrow your pen, not the ownership of your pen :-)\n>\n> > >>> +\n> > >>> + * the owner guarantees the validity of the reference for the whole duration\n> > >>> + of borrowing, and\n> > >>> + * the borrower doesn't access the reference after the end of the borrowing.\n> > >>> +\n> > >>> + When borrowing from caller to callee for the duration of a function call,\n> > >>\n> > >> \"When borrowing from caller to callee\"\n> > >> Can you borrow from callee to caller?\n> > >\n> > > Yes you can. CameraManager::instance() does exactly that, it returns a\n> > > reference that is still owned by CameraManager can can be borrowed for\n> > > the duration of the libcamera lifetime (which should be long enough :-)).\n> > >\n> >\n> > Right!\n> >\n> > >>> + this implies that the callee shall not keep any stored reference after it\n> > >>> + returns. These rules applies to the callee and all the functions it calls,\n> > >>\n> > >> rules -> apply\n> > >\n> > > Fixed.\n> > >\n> > >>> + directly or indirectly.\n> > >>\n> > >> * Ownership is borrowed by accessing the unique_ptr<> owned resources\n> > >> in the caller and by declaring the callee function parameter as:\n> > >> Your two points below here (const &) and pointer.\n> > >> * const &\n> > >> * pointers.\n> >\n> > Does this point not apply in your opinion?\n>\n> I missed it, sorry. I'll take this into account in v2.\n>\n> > >>> + * When the object doesn't need to be modified and may not be null, borrowed\n> > >>> + references are passed as 'const &'.\n> > >>> + * When the object may be modified or can be null, borrowed references are\n> > >>> + passed as pointers. Unless otherwise specified, pointers passed to\n> > >>> + functions are considered as borrowed references valid for the duration of\n> > >>> + the function only.\n> > >>> + * Single ownership is emphasized as much as possible by storing the unique\n> > >>> + reference as a std::unique_ptr<>.\n> > >>\n> > >> Shouldn't this be (one of) the first points?\n> > >>\n> > >>> + * Ownership is transfered by passing the reference as a std::unique_ptr<>.\n> > >>\n> > >> s/transfered/transferred\n> > >\n> > > The spelling \"transfered\" exists:\n> > > https://en.wiktionary.org/wiki/transfered. I'll still fix it :-)\n> > >\n> >\n> > Ah! my spell checker complained and I didn't look it up online\n>\n> You spellchecker rightfully complained, as \"transfered\" is documented as\n> \"Misspelling of transferred\" :-)\n>\n> > >> * Ownership is transfered by passing the reference as a std::unique_ptr<>.\n> > >> or\n> > >> * Ownership is transferred by declaring the callee functions\n> > >> parameter as std::unique_ptr<>\n> > >> (I would use the \"by declaring the callee function parameter\n> > >> as\" in the description of owership borrowing too)\n> > >\n> > > How about \"Ownership transfer is expressed by passing the reference as a\n> > > std::unique_ptr<>.\" ? I'd like to avoid referring to function calls\n> > > here, as that's not the only way to transfer ownership:\n> > >\n> > > struct Foo\n> > > {\n> > > std::unique_ptr ptr;\n> > > };\n> > >\n> > > Foo a;\n> > > Foo b;\n> > >\n> > > b.ptr = std::move(a.ptr);\n> > >\n> >\n> > I see... I like your suggestion...\n> >\n> > > (This will likely not appear as-is in our code of course.)\n> >\n> > Hopefully not :)\n> >\n> > >\n> > >> Also, the use of std::unique_ptr<> as function argument implies the object\n> > >> ownership gets moved to the callee so it won't be valid in the caller\n> > >> afterwards (if not returned by the callee and re-assigned again in the\n> > >> caller). Is this worth being described or is it clear enough in the\n> > >> definition of what a unique_ptr<> is?\n> > >\n> > > I think it's clear from the std::unique_ptr<> definition and the concept\n> > > of ownership transfer, but I can add the following sentence if you think\n> > > it could be useful.\n> > >\n> > > \"After ownership transfer the former owner has no valid reference to the\n> > > object anymore and shall not access it without obtaining a valid\n> > > reference.\"\n> >\n> > I let you decide if it is worth or not\n>\n> I'll add it.\n>\n> > >> Furthermore, do you think moving ownership of a resource to a callee\n> > >> function should be marked as a corner case, or indication of an issue\n> > >> in the design, or do you see this happening often in practice?\n> > >\n> > > I think it can happen. See the setEventDispatcher() patch for instance.\n> > > The function documentation states that the CameraManager takes ownership\n> > > of the dispatcher, but with the patch the std::unique_ptr<> in the\n> > > caller becomes null, ensuring that the caller will not erroneously try\n> > > to delete the dispatcher later on.\n> >\n> > and hopefully won't try to access it :)\n> >\n> > >>> +\n> > >>> +#. Shared Objects\n> > >>> +\n> > >>> + * Objects that may have multiple owners at a given time are called shared\n> > >>> + objects. They are reference-counted and live as long as any references to\n> > >>> + the object exist.\n> > >>> + * Shared objects are created with std::make_shared<> or\n> > >>> + std::allocate_shared<> an stored in an std::shared_ptr<>.\n> > >>> + * Borrowed references to shared objects are passed with the same rules as for\n> > >>> + single owner objects.\n> > >>\n> > >> Yes, but in this case 'borrowing' and 'transferring' of ownership have\n> > >> different side effects on the object reference counting. I would\n> > >> state that explicitly.\n> > >\n> > > Note that borrowing a reference to an object managed through a shared\n> > > pointer is done by passing a reference to the object itself, not the\n> > > shared pointer. I'll clarify this:\n> > >\n> > > * Borrowed references to shared objects are passed as references to the\n> > > object itself, not to the std::shared_ptr<>, with the same rules as for\n> > > single owner objects.\n> >\n> > Why can't you borrow a reference as \"shared_ptr<> &\" ? That's\n> > effectively borrowing a shared reference without increasing the\n> > reference count, isn't it?\n>\n> You can do that too, but it's more complex. Functions that borrow a\n> reference shouldn't be written assuming that the object uses shared_ptr,\n> unique_ptr or no smart pointer, they should take a reference or pointer\n> to the object directly.\n\nI'd vote against passing shared_ptr by reference (i.e. \"shared_ptr<>\n&\"). Semantically it's more clear if we always pass shared_ptr by\nvalue. The caller can decide whether it wants to duplicate the shared\nownership (through copying the shared_ptr), or transferring its own\nshare of the ownership to the callee (through std::move the\nshared_ptr).\n\nSome reference:\n\nhttps://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#object-ownership-and-calling-conventions\n\nI understand we're following Google C++ style guide, but I find the\nChromium C++ style guide useful as well. In the link scoped_refptr\nis the Chromium's version of std::shared_ptr.\n\n>\n> > >>> + * Ownership is shared by creating and passing copies of any valid\n> > >>> + std::shared_ptr<> reference. Ownership is released by destroying the\n> > >>> + corresponding std::shared_ptr<>.\n> > >>> +\n> > >>> +.. attention:: Long term borrowing of single owner objects is allowed. Example\n> > >>> + use cases are implementation of the singleton pattern (where the singleton\n> > >>> + guarantees the validity of the reference forever), or returning references\n> > >>> + to global objects whose lifetime matches the lifetime of the application. As\n> > >>> + long term borrowing isn't marked through language constructs, it shall be\n> > >>> + documented explicitly in details in the API.\n> > >>\n> > >> Thanks, this note is very useful imho.\n> > >> Feel free to take in whatever you consider appropriate\n> > >\n> > > Thank you for your comments. Please let me know if you're fine with the\n> > > changes I proposed above, and which ones should be included or dropped.\n> > >\n> >\n> > I'm fine, please go ahead merging this once you consider it ready.\n> >\n> > >>> +\n> > >>>\n> > >>> Tools\n> > >>> -----\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from mail-vk1-xa44.google.com (mail-vk1-xa44.google.com\n\t[IPv6:2607:f8b0:4864:20::a44])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 59ACA60B21\n\tfor ;\n\tFri, 18 Jan 2019 18:54:08 +0100 (CET)","by mail-vk1-xa44.google.com with SMTP id o130so3251542vke.10\n\tfor ;\n\tFri, 18 Jan 2019 09:54:08 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=fyaw2ScOM4zYr237iR7kecAXQoJ0uwnLqFv0VkbH8g0=;\n\tb=a7LiSqOmMd/IQ6FdZI+7VVicFfOIu3gl6U/s3PWVnR3f0DmYzhgtNbqXz0LNkwOI9x\n\tol3/4JRitX834WhPAms4zXaqeo+/+bV5ji5zASGr2upvgfwHX3MaFpMaSYnc0+R2RziR\n\tr7cZCp8Sm2SyZNtKn/Kk1R7GF68JTqhUMKUUA=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=fyaw2ScOM4zYr237iR7kecAXQoJ0uwnLqFv0VkbH8g0=;\n\tb=UkgsYw7SNPqi+NEoKkvXTkgqoXSOqzf+HxQPWwV8HFqaUUNNcJ3tT30KSCCadLYqub\n\trqQsAxCdjFdCddMGVOzrlssHfX83xsLZkoc2Cl7bs5XiK3bRTEfMnsVLseF144HqjQZJ\n\t8dUcrOx6ou8SkeBh0laZ70VvD/UWZCeLyJtUGNR6XPlrhz8E/ehvaA1qD4KYQtENAKMZ\n\tFHScgUJyWF8bp/sIAQ//H7/xX1OPUYDytQyTtvYcbXEO9c7lVnLIU8OlZSmheB8iEcg6\n\tLNIy53CQr92UlNZ30dLILOfYrMZUlXQl07H/TzeDPgynWbT9ASc7iTSTT4lO7aXVj/tQ\n\tZabA==","X-Gm-Message-State":"AJcUukc7ou7TAs5Agn+YJ1dTu8SchdBDwMZ6k44efLQAsMaNsM08DuWl\n\t6BNNLlmYPHcrb+xgfgScSbMT/vgWB/ucN23vwssTmg==","X-Google-Smtp-Source":"ALg8bN5soqPmNPJtJ0Ha1hxl5u/ibFP5GWxQpAQiJcgAAquy+RE1OhsEMAmZ49HlPFLrJDxIaB9g8b6Pt6dBav60EII=","X-Received":"by 2002:a1f:6b14:: with SMTP id g20mr7730867vkc.47.1547834015413;\n\tFri, 18 Jan 2019 09:53:35 -0800 (PST)","MIME-Version":"1.0","References":"<20190117235916.1906-1-laurent.pinchart@ideasonboard.com>\n\t<20190117235916.1906-2-laurent.pinchart@ideasonboard.com>\n\t<20190118145608.qrypk6zilehgmk7m@uno.localdomain>\n\t<20190118161719.GI5275@pendragon.ideasonboard.com>\n\t<20190118170044.ae4e77kvn6sc5yog@uno.localdomain>\n\t<20190118174131.GA1799@pendragon.ideasonboard.com>","In-Reply-To":"<20190118174131.GA1799@pendragon.ideasonboard.com>","From":"Ricky Liang ","Date":"Sat, 19 Jan 2019 01:53:15 +0800","Message-ID":"","To":"Laurent Pinchart ","Cc":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 1/4] Documentation: coding_style: Add\n\tobject ownership rules","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 18 Jan 2019 17:54:08 -0000"}},{"id":427,"web_url":"https://patchwork.libcamera.org/comment/427/","msgid":"<20190118232316.GD1799@pendragon.ideasonboard.com>","date":"2019-01-18T23:23:16","subject":"Re: [libcamera-devel] [PATCH 1/4] Documentation: coding_style: Add\n\tobject ownership rules","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Ricky,\n\nOn Sat, Jan 19, 2019 at 01:53:15AM +0800, Ricky Liang wrote:\n> On Sat, Jan 19, 2019 at 1:41 AM Laurent Pinchart\n> wrote:\n> > On Fri, Jan 18, 2019 at 06:00:44PM +0100, Jacopo Mondi wrote:\n> >> On Fri, Jan 18, 2019 at 06:17:19PM +0200, Laurent Pinchart wrote:\n> >>> On Fri, Jan 18, 2019 at 03:56:08PM +0100, Jacopo Mondi wrote:\n> >>>> Hi Laurent,\n> >>>> thanks for doing this!\n> >>>> \n> >>>> I don't look picky here, but this is an important piece of\n> >>>> documentation, and I want to make sure I got it right first...\n> >>> \n> >>> This is exactly the kind of patch for which I expect picky reviews :-)\n> >>> \n> >> \n> >> Happy to be of any help then :)\n> >> \n> >>>> On Fri, Jan 18, 2019 at 01:59:13AM +0200, Laurent Pinchart wrote:\n> >>>>> Object ownership is a complex topic that can lead to many issues, from\n> >>>>> memory leak to crashes. Document the rules that libcamera enforces to\n> >>>>> make object ownership tracking explicit.\n> >>>>> \n> >>>>> This is a first version of the rules and is expected to be expanded as\n> >>>>> the library is developed.\n> >>>>> \n> >>>>> Signed-off-by: Laurent Pinchart \n> >>>>> ---\n> >>>>> Documentation/coding-style.rst | 62 ++++++++++++++++++++++++++++++++++\n> >>>>> 1 file changed, 62 insertions(+)\n> >>>>> \n> >>>>> diff --git a/Documentation/coding-style.rst b/Documentation/coding-style.rst\n> >>>>> index f8d2fdfeda8e..f77325239bfa 100644\n> >>>>> --- a/Documentation/coding-style.rst\n> >>>>> +++ b/Documentation/coding-style.rst\n> >>>>> @@ -81,6 +81,68 @@ C++-11-specific features:\n> >>>>> overused.\n> >>>>> * Variadic class and function templates\n> >>>>> \n> >>>>> +Object Ownership\n> >>>>> +~~~~~~~~~~~~~~~~\n> >>>>> +\n> >>>>> +libcamera creates and destroys many objects at runtime, for both objects\n> >>>>> +internal to the library and objects exposed to the user. To guarantee proper\n> >>>>> +operation without use after free, double free or memory leaks, knowing who owns\n> >>>>> +each object at any time is crucial. The project has enacted a set of rules to\n> >>>>> +make object ownership tracking as explicit and fool-proof as possible.\n> >>>>> +\n> >>>>> +In the context of this section, the terms object and instance are used\n> >>>>> +interchangeably and both refer to an instance of a class. The term reference\n> >>>>> +refers to both C++ references and C++ pointers in their capacity to refer to an\n> >>>>> +object. Passing a reference means offering a way to a callee to obtain a\n> >>>>> +reference to an object that the caller has a valid reference to. Borrowing a\n> >>>>> +reference means using a reference passed by a caller without ownership transfer\n> >>>>> +based on the assumption that the caller guarantees the validate of the\n> >>>> \n> >>>> the validity\n> >>> \n> >>> Fixed.\n> >>> \n> >>>>> +reference for the duration of the operation that borrows the reference.\n> >>>> \n> >>>> maybe just \"that borrows it.\"\n> >>> \n> >>> Fixed.\n> >>> \n> >>>>> +\n> >>>>> +#. Single Owner Objects\n> >>>>> +\n> >>>>> + * By default an object has a single owner at any time.\n> >>>> \n> >>>> \"an object that has a\"\n> >>> \n> >>> No, I really meant to say that by default an object has a single owner\n> >>> at any time, to explain that by default objects do not use shared\n> >>> ownership.\n> >>> \n> >> \n> >> Ah right, I thougth this was the definition of what a \"single owner\n> >> object\" is...\n> >> \n> >> \n> >>>>> + * References to a single owner object can be borrowed by passing them from\n> >>>>> + the owner to the borrower, providing that\n> >>>> \n> >>>> Not sure this was what you wanted to express, but it seems to that\n> >>>> it would be more clear to state that \"Ownership of a single owner\n> >>>> object can be borrowed, providing that\"\n> >>> \n> >>> But then I can't use \"reference\" in the list below. How about\n> >>> \n> >>> \"A single owner object can be borrowed by passing a reference from the\n> >>> owner to the borrower, providing that\"\n> >>> \n> >> I still feel like it is actually the ownership that is\n> >> transferred/moved not the reference itself. This is fine anyway!\n> > \n> > When transferring ownership, you're right, but when borrowing the\n> > object, it's the object that you borrow, not its ownership. I'll ask you\n> > to borrow your pen, not the ownership of your pen :-)\n> > \n> >>>>> +\n> >>>>> + * the owner guarantees the validity of the reference for the whole duration\n> >>>>> + of borrowing, and\n> >>>>> + * the borrower doesn't access the reference after the end of the borrowing.\n> >>>>> +\n> >>>>> + When borrowing from caller to callee for the duration of a function call,\n> >>>> \n> >>>> \"When borrowing from caller to callee\"\n> >>>> Can you borrow from callee to caller?\n> >>> \n> >>> Yes you can. CameraManager::instance() does exactly that, it returns a\n> >>> reference that is still owned by CameraManager can can be borrowed for\n> >>> the duration of the libcamera lifetime (which should be long enough :-)).\n> >>> \n> >> \n> >> Right!\n> >> \n> >>>>> + this implies that the callee shall not keep any stored reference after it\n> >>>>> + returns. These rules applies to the callee and all the functions it calls,\n> >>>> \n> >>>> rules -> apply\n> >>> \n> >>> Fixed.\n> >>> \n> >>>>> + directly or indirectly.\n> >>>> \n> >>>> * Ownership is borrowed by accessing the unique_ptr<> owned resources\n> >>>> in the caller and by declaring the callee function parameter as:\n> >>>> Your two points below here (const &) and pointer.\n> >>>> * const &\n> >>>> * pointers.\n> >> \n> >> Does this point not apply in your opinion?\n> > \n> > I missed it, sorry. I'll take this into account in v2.\n> > \n> >>>>> + * When the object doesn't need to be modified and may not be null, borrowed\n> >>>>> + references are passed as 'const &'.\n> >>>>> + * When the object may be modified or can be null, borrowed references are\n> >>>>> + passed as pointers. Unless otherwise specified, pointers passed to\n> >>>>> + functions are considered as borrowed references valid for the duration of\n> >>>>> + the function only.\n> >>>>> + * Single ownership is emphasized as much as possible by storing the unique\n> >>>>> + reference as a std::unique_ptr<>.\n> >>>> \n> >>>> Shouldn't this be (one of) the first points?\n> >>>> \n> >>>>> + * Ownership is transfered by passing the reference as a std::unique_ptr<>.\n> >>>> \n> >>>> s/transfered/transferred\n> >>> \n> >>> The spelling \"transfered\" exists:\n> >>> https://en.wiktionary.org/wiki/transfered. I'll still fix it :-)\n> >>> \n> >> \n> >> Ah! my spell checker complained and I didn't look it up online\n> > \n> > You spellchecker rightfully complained, as \"transfered\" is documented as\n> > \"Misspelling of transferred\" :-)\n> > \n> >>>> * Ownership is transfered by passing the reference as a std::unique_ptr<>.\n> >>>> or\n> >>>> * Ownership is transferred by declaring the callee functions\n> >>>> parameter as std::unique_ptr<>\n> >>>> (I would use the \"by declaring the callee function parameter\n> >>>> as\" in the description of owership borrowing too)\n> >>> \n> >>> How about \"Ownership transfer is expressed by passing the reference as a\n> >>> std::unique_ptr<>.\" ? I'd like to avoid referring to function calls\n> >>> here, as that's not the only way to transfer ownership:\n> >>> \n> >>> struct Foo\n> >>> {\n> >>> std::unique_ptr ptr;\n> >>> };\n> >>> \n> >>> Foo a;\n> >>> Foo b;\n> >>> \n> >>> b.ptr = std::move(a.ptr);\n> >>> \n> >> \n> >> I see... I like your suggestion...\n> >> \n> >>> (This will likely not appear as-is in our code of course.)\n> >> \n> >> Hopefully not :)\n> >> \n> >>> \n> >>>> Also, the use of std::unique_ptr<> as function argument implies the object\n> >>>> ownership gets moved to the callee so it won't be valid in the caller\n> >>>> afterwards (if not returned by the callee and re-assigned again in the\n> >>>> caller). Is this worth being described or is it clear enough in the\n> >>>> definition of what a unique_ptr<> is?\n> >>> \n> >>> I think it's clear from the std::unique_ptr<> definition and the concept\n> >>> of ownership transfer, but I can add the following sentence if you think\n> >>> it could be useful.\n> >>> \n> >>> \"After ownership transfer the former owner has no valid reference to the\n> >>> object anymore and shall not access it without obtaining a valid\n> >>> reference.\"\n> >> \n> >> I let you decide if it is worth or not\n> > \n> > I'll add it.\n> > \n> >>>> Furthermore, do you think moving ownership of a resource to a callee\n> >>>> function should be marked as a corner case, or indication of an issue\n> >>>> in the design, or do you see this happening often in practice?\n> >>> \n> >>> I think it can happen. See the setEventDispatcher() patch for instance.\n> >>> The function documentation states that the CameraManager takes ownership\n> >>> of the dispatcher, but with the patch the std::unique_ptr<> in the\n> >>> caller becomes null, ensuring that the caller will not erroneously try\n> >>> to delete the dispatcher later on.\n> >> \n> >> and hopefully won't try to access it :)\n> >> \n> >>>>> +\n> >>>>> +#. Shared Objects\n> >>>>> +\n> >>>>> + * Objects that may have multiple owners at a given time are called shared\n> >>>>> + objects. They are reference-counted and live as long as any references to\n> >>>>> + the object exist.\n> >>>>> + * Shared objects are created with std::make_shared<> or\n> >>>>> + std::allocate_shared<> an stored in an std::shared_ptr<>.\n> >>>>> + * Borrowed references to shared objects are passed with the same rules as for\n> >>>>> + single owner objects.\n> >>>> \n> >>>> Yes, but in this case 'borrowing' and 'transferring' of ownership have\n> >>>> different side effects on the object reference counting. I would\n> >>>> state that explicitly.\n> >>> \n> >>> Note that borrowing a reference to an object managed through a shared\n> >>> pointer is done by passing a reference to the object itself, not the\n> >>> shared pointer. I'll clarify this:\n> >>> \n> >>> * Borrowed references to shared objects are passed as references to the\n> >>> object itself, not to the std::shared_ptr<>, with the same rules as for\n> >>> single owner objects.\n> >> \n> >> Why can't you borrow a reference as \"shared_ptr<> &\" ? That's\n> >> effectively borrowing a shared reference without increasing the\n> >> reference count, isn't it?\n> > \n> > You can do that too, but it's more complex. Functions that borrow a\n> > reference shouldn't be written assuming that the object uses shared_ptr,\n> > unique_ptr or no smart pointer, they should take a reference or pointer\n> > to the object directly.\n> \n> I'd vote against passing shared_ptr by reference (i.e. \"shared_ptr<>\n> &\"). Semantically it's more clear if we always pass shared_ptr by\n> value. The caller can decide whether it wants to duplicate the shared\n> ownership (through copying the shared_ptr), or transferring its own\n> share of the ownership to the callee (through std::move the\n> shared_ptr).\n\nI think that makes sense, I'll add a bullet point to mention this:\n\n * When passed to a function, std::shared_ptr<> are always passed by value, \n never by reference. The caller can decide whether to transfer its ownership\n of the std::shared_ptr<> with std::move() or retain it. The callee shall \n use std::move() if it needs to store the shared pointer.\n\n> Some reference:\n> \n> https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#object-ownership-and-calling-conventions\n\nThank you for the link. I'll add it to the documentation.\n\n> I understand we're following Google C++ style guide, but I find the\n> Chromium C++ style guide useful as well. In the link scoped_refptr\n> is the Chromium's version of std::shared_ptr.\n\nWe follow the Google C++ style guide as a base because we didn't feel\nlike writing a full style guide from scratch :-) We're certainly open to\nadapting some of the Chromium-specific style rules when it makes sense,\nsuch as the ones about object ownership.\n\n> >>>>> + * Ownership is shared by creating and passing copies of any valid\n> >>>>> + std::shared_ptr<> reference. Ownership is released by destroying the\n> >>>>> + corresponding std::shared_ptr<>.\n> >>>>> +\n> >>>>> +.. attention:: Long term borrowing of single owner objects is allowed. Example\n> >>>>> + use cases are implementation of the singleton pattern (where the singleton\n> >>>>> + guarantees the validity of the reference forever), or returning references\n> >>>>> + to global objects whose lifetime matches the lifetime of the application. As\n> >>>>> + long term borrowing isn't marked through language constructs, it shall be\n> >>>>> + documented explicitly in details in the API.\n> >>>> \n> >>>> Thanks, this note is very useful imho.\n> >>>> Feel free to take in whatever you consider appropriate\n> >>> \n> >>> Thank you for your comments. Please let me know if you're fine with the\n> >>> changes I proposed above, and which ones should be included or dropped.\n> >>> \n> >> \n> >> I'm fine, please go ahead merging this once you consider it ready.\n> >> \n> >>>>> +\n> >>>>> \n> >>>>> Tools\n> >>>>> -----","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1440960C78\n\tfor ;\n\tSat, 19 Jan 2019 00:23:17 +0100 (CET)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2054B53E;\n\tSat, 19 Jan 2019 00:23:16 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1547853796;\n\tbh=PHcEgTMvsF+09oQwQSuILnh4VROoarXA0SdtS4NtOiM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=NPV3QccyRKN3ahdZwqnjrwCujHVbh2QGnPsKkV3FKXNgfUfID0drmGCbifA4wn2pa\n\t3jVmrTER1r0lK3UgJMjhsKnUVhO+JIBim+KbL0rc7g9HC4OhcONqWUtfe9zSjFNH81\n\tSqxiy9JbcVoM1DjZTLZhNAzcsUgWOCPxSwB7y3gw=","Date":"Sat, 19 Jan 2019 01:23:16 +0200","From":"Laurent Pinchart ","To":"Ricky Liang ","Cc":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","Message-ID":"<20190118232316.GD1799@pendragon.ideasonboard.com>","References":"<20190117235916.1906-1-laurent.pinchart@ideasonboard.com>\n\t<20190117235916.1906-2-laurent.pinchart@ideasonboard.com>\n\t<20190118145608.qrypk6zilehgmk7m@uno.localdomain>\n\t<20190118161719.GI5275@pendragon.ideasonboard.com>\n\t<20190118170044.ae4e77kvn6sc5yog@uno.localdomain>\n\t<20190118174131.GA1799@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 1/4] Documentation: coding_style: Add\n\tobject ownership rules","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 18 Jan 2019 23:23:17 -0000"}}]