Re: [PATCH] media: Document coding style requirements

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Jacopo,

Thanks for the patch.

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.

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

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

-- 
Kind regards,

Sakari Ailus