[libcamera-devel] [PATCH 01/38] Documentation: coding-style: Document global variable guidelines

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Wed, Sep 23, 2020 at 12:13:16PM +0200, Jacopo Mondi wrote:
> On Tue, Sep 22, 2020 at 10:35:00PM +0900, Paul Elder wrote:
> > Document the issue related to global variable dependencies and how to
> > avoid them.
> >
> > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> >
> > ---
> > New in v2
> > ---
> >  Documentation/coding-style.rst | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> >
> > diff --git a/Documentation/coding-style.rst b/Documentation/coding-style.rst
> > index 8af06d6a..967506db 100644
> > --- a/Documentation/coding-style.rst
> > +++ b/Documentation/coding-style.rst
> > @@ -187,6 +187,21 @@ These rules match the `object ownership rules from the Chromium C++ Style Guide`
> >     long term borrowing isn't marked through language constructs, it shall be
> >     documented explicitly in details in the API.
> >
> > +Global Variables
> > +~~~~~~~~~~~~~~~~
> > +
> > +The order of initialization and destructions of global variables cannot be
> > +reasonably controlled. This can cause problems (segfaults) when global

s/segfaults/including segfaults/ as there can be other types of
problems.

> > +variables depend on each other, or when non-globals depend on globals.

The latter isn't possible, at least directly, as all global variables
will be constructed before anything else happens. I would write "depend
on each other, directly or indirectly".

> > +For example, if the declaration of a global variable calls a constructor,
> > +which calls into another global variable that has yet to be initialized, that

"calling into a variable" sounds weird. Maybe "which uses another global
variable that hasn't been initialized yet" ?

> > +is likely to segfault.

As there are other possible issues than segfaults, "[...], incorrect
behaviour is likely.".

And I would add "Similar issues may occur when the library is unloaded
and global variables are destroyed."

> > +
> > +The best solution is to avoid global variables when possible. If required,

Not all of them though, there's no need to avoid global variables that
have constexpr constructors and trivial destructors. I would write

Global variables that are statically initialized and have trivial destructors
(such as an integer constant for instance) do no cause any issue. Other global
variables shall be avoided when possible, but are allowed when required (for
instance to implement factories with auto-registration). They shall no depend
on any other global variable, by running ...".

> > +such as for implementing factories with auto-registration, avoid dependencies
> > +by running the minimum amount of code in the constructor and destructor,
> > +and move code that contains dependencies to a later point of time.
> > +
> 
> While avoiding globals is desirable, sometimes it makes things easier,
> and if the type of the variable is trivially destructible and has a

It should be trivially constructible too :-) Or at least the constructor
and destructor should not call into other global objects.

> constexpr constructor we should be safe against
> initialization/destruction order failures.
> 
> This section is fine as it is a 'stay safe' kind-of paragraph, but if
> you feel like expanding it you can have some references from here
> https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables

Good point. We have less strict rules though.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> >  C Compatibility Headers
> >  ~~~~~~~~~~~~~~~~~~~~~~~
> >

-- 
Regards,

Laurent Pinchart