[{"id":12657,"web_url":"https://patchwork.libcamera.org/comment/12657/","msgid":"<20200923101316.hww5yho27yxwmdej@uno.localdomain>","date":"2020-09-23T10:13:16","subject":"Re: [libcamera-devel] [PATCH 01/38] Documentation: coding-style:\n\tDocument global variable guidelines","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Paul,\n\nOn Tue, Sep 22, 2020 at 10:35:00PM +0900, Paul Elder wrote:\n> Document the issue related to global variable dependencies and how to\n> avoid them.\n>\n> Signed-off-by: Paul Elder \n>\n> ---\n> New in v2\n> ---\n> Documentation/coding-style.rst | 15 +++++++++++++++\n> 1 file changed, 15 insertions(+)\n>\n> diff --git a/Documentation/coding-style.rst b/Documentation/coding-style.rst\n> index 8af06d6a..967506db 100644\n> --- a/Documentation/coding-style.rst\n> +++ b/Documentation/coding-style.rst\n> @@ -187,6 +187,21 @@ These rules match the `object ownership rules from the Chromium C++ Style Guide`\n> long term borrowing isn't marked through language constructs, it shall be\n> documented explicitly in details in the API.\n>\n> +Global Variables\n> +~~~~~~~~~~~~~~~~\n> +\n> +The order of initialization and destructions of global variables cannot be\n> +reasonably controlled. This can cause problems (segfaults) when global\n> +variables depend on each other, or when non-globals depend on globals.\n> +For example, if the declaration of a global variable calls a constructor,\n> +which calls into another global variable that has yet to be initialized, that\n> +is likely to segfault.\n> +\n> +The best solution is to avoid global variables when possible. If required,\n> +such as for implementing factories with auto-registration, avoid dependencies\n> +by running the minimum amount of code in the constructor and destructor,\n> +and move code that contains dependencies to a later point of time.\n> +\n\nWhile avoiding globals is desirable, sometimes it makes things easier,\nand if the type of the variable is trivially destructible and has a\nconstexpr constructor we should be safe against\ninitialization/destruction order failures.\n\nThis section is fine as it is a 'stay safe' kind-of paragraph, but if\nyou feel like expanding it you can have some references from here\nhttps://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables\n\nThanks\n j\n\n\n> C Compatibility Headers\n> ~~~~~~~~~~~~~~~~~~~~~~~\n>\n> --\n> 2.27.0\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 6B6A0C3B5C\n\tfor ;\n\tWed, 23 Sep 2020 10:09:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D908A62FE1;\n\tWed, 23 Sep 2020 12:09:25 +0200 (CEST)","from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C72DE62FD2\n\tfor ;\n\tWed, 23 Sep 2020 12:09:24 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay4-d.mail.gandi.net (Postfix) with ESMTPSA id 49F77E0006;\n\tWed, 23 Sep 2020 10:09:23 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Wed, 23 Sep 2020 12:13:16 +0200","From":"Jacopo Mondi ","To":"Paul Elder ","Message-ID":"<20200923101316.hww5yho27yxwmdej@uno.localdomain>","References":"<20200922133537.258098-1-paul.elder@ideasonboard.com>\n\t<20200922133537.258098-2-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200922133537.258098-2-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 01/38] Documentation: coding-style:\n\tDocument global variable guidelines","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12858,"web_url":"https://patchwork.libcamera.org/comment/12858/","msgid":"<20200929033807.GX14614@pendragon.ideasonboard.com>","date":"2020-09-29T03:38:07","subject":"Re: [libcamera-devel] [PATCH 01/38] Documentation: coding-style:\n\tDocument global variable guidelines","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Wed, Sep 23, 2020 at 12:13:16PM +0200, Jacopo Mondi wrote:\n> On Tue, Sep 22, 2020 at 10:35:00PM +0900, Paul Elder wrote:\n> > Document the issue related to global variable dependencies and how to\n> > avoid them.\n> >\n> > Signed-off-by: Paul Elder \n> >\n> > ---\n> > New in v2\n> > ---\n> > Documentation/coding-style.rst | 15 +++++++++++++++\n> > 1 file changed, 15 insertions(+)\n> >\n> > diff --git a/Documentation/coding-style.rst b/Documentation/coding-style.rst\n> > index 8af06d6a..967506db 100644\n> > --- a/Documentation/coding-style.rst\n> > +++ b/Documentation/coding-style.rst\n> > @@ -187,6 +187,21 @@ These rules match the `object ownership rules from the Chromium C++ Style Guide`\n> > long term borrowing isn't marked through language constructs, it shall be\n> > documented explicitly in details in the API.\n> >\n> > +Global Variables\n> > +~~~~~~~~~~~~~~~~\n> > +\n> > +The order of initialization and destructions of global variables cannot be\n> > +reasonably controlled. This can cause problems (segfaults) when global\n\ns/segfaults/including segfaults/ as there can be other types of\nproblems.\n\n> > +variables depend on each other, or when non-globals depend on globals.\n\nThe latter isn't possible, at least directly, as all global variables\nwill be constructed before anything else happens. I would write \"depend\non each other, directly or indirectly\".\n\n> > +For example, if the declaration of a global variable calls a constructor,\n> > +which calls into another global variable that has yet to be initialized, that\n\n\"calling into a variable\" sounds weird. Maybe \"which uses another global\nvariable that hasn't been initialized yet\" ?\n\n> > +is likely to segfault.\n\nAs there are other possible issues than segfaults, \"[...], incorrect\nbehaviour is likely.\".\n\nAnd I would add \"Similar issues may occur when the library is unloaded\nand global variables are destroyed.\"\n\n> > +\n> > +The best solution is to avoid global variables when possible. If required,\n\nNot all of them though, there's no need to avoid global variables that\nhave constexpr constructors and trivial destructors. I would write\n\nGlobal variables that are statically initialized and have trivial destructors\n(such as an integer constant for instance) do no cause any issue. Other global\nvariables shall be avoided when possible, but are allowed when required (for\ninstance to implement factories with auto-registration). They shall no depend\non any other global variable, by running ...\".\n\n> > +such as for implementing factories with auto-registration, avoid dependencies\n> > +by running the minimum amount of code in the constructor and destructor,\n> > +and move code that contains dependencies to a later point of time.\n> > +\n> \n> While avoiding globals is desirable, sometimes it makes things easier,\n> and if the type of the variable is trivially destructible and has a\n\nIt should be trivially constructible too :-) Or at least the constructor\nand destructor should not call into other global objects.\n\n> constexpr constructor we should be safe against\n> initialization/destruction order failures.\n> \n> This section is fine as it is a 'stay safe' kind-of paragraph, but if\n> you feel like expanding it you can have some references from here\n> https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables\n\nGood point. We have less strict rules though.\n\nReviewed-by: Laurent Pinchart \n\n> > C Compatibility Headers\n> > ~~~~~~~~~~~~~~~~~~~~~~~\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 0B69BC3B5B\n\tfor ;\n\tTue, 29 Sep 2020 03:38:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6E129620BC;\n\tTue, 29 Sep 2020 05:38:44 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7F63B60367\n\tfor ;\n\tTue, 29 Sep 2020 05:38:42 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E7167540;\n\tTue, 29 Sep 2020 05:38:41 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"eEyrDBtk\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1601350722;\n\tbh=vxkG38AxRNeQMC0xbiR0oRcJu2Xj+DCqSgs/Wl1mhFY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=eEyrDBtkwILGDW8xXxMEeUS9PJvpgkWwSzR7/IZMJiedk2xhXdIjmV66sSLBb2QRa\n\tCIiFim1rcp6Wzz7BoGJH5ifIQyBvaNiRG/YSSnww65ddnTbLFivzGiP4BpB6RRvxOy\n\tqXgt6Xtc8RO3AAyLyJYjpvWDr1RWKOTd8RZlf2fA=","Date":"Tue, 29 Sep 2020 06:38:07 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Message-ID":"<20200929033807.GX14614@pendragon.ideasonboard.com>","References":"<20200922133537.258098-1-paul.elder@ideasonboard.com>\n\t<20200922133537.258098-2-paul.elder@ideasonboard.com>\n\t<20200923101316.hww5yho27yxwmdej@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200923101316.hww5yho27yxwmdej@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 01/38] Documentation: coding-style:\n\tDocument global variable guidelines","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]