Re: [PATCH] media: Document coding style requirements

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Sakari,

On Tue, Oct 19, 2021 at 12:24:41PM +0300, Sakari Ailus wrote:
> On Wed, Oct 13, 2021 at 11:20:05AM +0200, Jacopo Mondi wrote:
> > There are a few additional coding style conventions in place in
> > the media subsystem. If they do not get documented, it's hard to enforce
> > them during review as well as it is hard for developers to follow them
> > without having previously contributed to the subsystem.
> > 
> > Add them to the subsystem profile documentation.
> > 
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> > 
> > All points are up for discussion ofc.
> > 
> > But the idea is to get to have more requirement defined, as otherwise
> > it's very hard to enforce them during review.
> 
> Thanks for the patch.
> 
> Aren't these all common and/or preferred practices outside the media tree
> as well? I suppose not each one of these is universally enforced though.

They're not I'm afraid :-) Different subsystems have different
preferences, and within the realm of what a subsystem allows, different
parts also use different coding style rules. It's the same for media,
depending on who maintains a set of drivers, the rules will be
different.

> The coding style guide is lacking documentation on such things though.

Trying to fix that with a top-down approach will in my opinion not work.
I'd rather focus on media first to see if we can do something at the
subsystem level, in a bottom-up way (I've even considered writing rules
specific to sensor drivers, but if we can reach an agreement at the
subsystem level, that would be better).

> > ---
> >  .../media/maintainer-entry-profile.rst        | 24 +++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> > 
> > diff --git a/Documentation/driver-api/media/maintainer-entry-profile.rst b/Documentation/driver-api/media/maintainer-entry-profile.rst
> > index eb1cdfd280ba..9c376f843e1c 100644
> > --- a/Documentation/driver-api/media/maintainer-entry-profile.rst
> > +++ b/Documentation/driver-api/media/maintainer-entry-profile.rst
> > @@ -180,6 +180,30 @@ In particular, we accept lines with more than 80 columns:
> >      - when they avoid a line to end with an open parenthesis or an open
> >        bracket.
> > 
> > +There are a few additional requirements which are not enforced by tooling
> > +but mostly during the review process:
> > +
> > +    - C++ style comments are not allowed, if not for SPDX headers;
> > +    - hexadecimal values should be spelled using lowercase letters;
> > +    - one structure/enum member declaration per line;
> > +    - one variable declaration per line;
> > +    - prefer variable declaration order in reverse-x-mas-tree over
> > +      initialization at variable declare time;
> > +
> > +      As an example, the following style is preferred::
> > +
> > +         struct priv_struct *priv = container_of(....)
> > +         struct foo_struct *foo = priv->foo;
> > +         int b;
> > +
> > +         b = a_very_long_operation_name(foo, s->bar)
> > +
> > +      over the following one::
> > +
> > +         struct priv_struct *priv = container_of(....)
> > +         struct foo_struct *foo = priv->foo;
> > +         int b = a_very_long_operation_name(foo, s->bar)
> 
> I wouldn't say this is required or even preferred if you have a dependency
> between the variables.
> 
> Rather I'd say the latter is undesirable if a_very_long_operation_name()
> can fail. But that's a bit out of scope now.
> 
> > +
> >  Key Cycle Dates
> >  ---------------

-- 
Regards,

Laurent Pinchart