Re: [PATCH] media: Document coding style requirements

[Jacopo Mondi <jacopo@jmondi.org>]

Hi Mauro,

On Thu, Oct 21, 2021 at 03:55:12PM +0100, Mauro Carvalho Chehab wrote:
> 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

Afaict C99 allowed // commenting style. We have to go back to C90 for
only /* */ :)

> line comments.
>

I wouldn't be against either, but I fear that without a slightly more
stringent rule we'll continue to have a lot of push-pull about what is
allowed or not.

I wouldn't be against something like "C++ comments are accepted for
single line comments" but as soon as someone uses them for multiple
lines we'll be back to discuss why 1 lines is fine 2 are not.
Execeptions also requires developers and reviewers to get back to the
subsystem rules for all tiny details. I feel one single rule, when possible,
would be simpler.

> >
> > > +    - 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
> > >
> >