Re: [PATCH] media: Document coding style requirements

[Mauro Carvalho Chehab <mchehab@kernel.org>]

Em Thu, 21 Oct 2021 16:00:40 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On 13/10/2021 11:20, 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
> >    j
> > 
> > ---
> >  .../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;  
> 
> if not -> except

While I prefer C99, I'm not really against having C++ comments on single
line comments. 

> 
> > +    - hexadecimal values should be spelled using lowercase letters;

> > +    - one structure/enum member declaration per line;
> > +    - one variable declaration per line;  
> 
> Hmm, I don't mind something like: int i, j;

I don't mind having things like:

	struct *dev, *parent_dev;

or even:

	struct *parent_dev, *dev = pdev->dev;

What it is really ugly is having multiple initialized vars at the
same declaration, like:

	struct *parent_dev = pdev->dev.parent, *dev = pdev->dev;

or, even worse:

	struct *dev = pdev->dev, *parent_dev = dev.parent;


> But for anything more complex I too prefer one declaration per line.
> 
> > +    - prefer variable declaration order in reverse-x-mas-tree over
> > +      initialization at variable declare time;  
> 
> Add something like:
> 
> ...unless there are dependencies or other readability reasons to
> depart from this.

+1

> 
> > +
> > +      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'm not sure if this is what you typically see.
> 
> Perhaps this is a better example:
> 
> 	int i;
> 	struct foo_struct *foo = priv->foo;
> 	int result;
> 
> should be written as:
> 
> 	struct foo_struct *foo = priv->foo;
> 	int result;
> 	int i;
> 
> Regards,
> 
> 	Hans
> 
> > +
> >  Key Cycle Dates
> >  ---------------
> > 
> > --
> > 2.33.0
> >   
>