| Message ID | 20211013092005.14268-1-jacopo@jmondi.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: Document coding style requirements | expand |
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 > ---------------
Hi Sakari, On Tue, Oct 19, 2021 at 12:24:41PM +0300, Sakari Ailus wrote: > 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. Apparently not :) Particularly, after the lifting of the 80-cols stringent limit, each subsystem has its own preferences on that regard (I'm not active in that many subsystem, but I've been recently asked not to break lines that goes up to 100 cols in a different subsystem, if I knew that from the beginning, I could have saved a v2 just to un-break lines). I find this very confusing, as each subsystem you interact with you're asked to comply with undocumented preferences. Hence, better document them here so that developers knows what comments they might expect to receive, and reviewers have ground to justify their requests. > > 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. > I'm fine to drop it, all the points are there mainly to sparkle discussions. But I think we should record something or decide that everything not prohibited is allowed (maybe that's the case already and I failed to understand it ?) I've been confronted with a "on what ground are you asking this ?" question recently when reviewing a driver commented only in C++ style. I got nothing to offer except the 'no other driver does that' which has proven not to be enough to convince the developer. Having a set of rules you can refer to would have helped (and would have saved me quite a few email exchanges that ended up in nothing). Please note this serves for the other way around too. If it get decided that everything that is not prohibited is allowed, I would have not made those comments in first place. So please do not interpret this as only directed to developers but also for less experienced reviewers to know what changes in style they can ask for. Thanks j > > + > > Key Cycle Dates > > --------------- > > -- > Kind regards, > > Sakari Ailus
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 > + - 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; 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. > + > + 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 >
On 21/10/2021 16:00, Hans Verkuil wrote: > 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 > >> + - 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; > > 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. > >> + >> + 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; There is one other requirement: the patches must be run through scripts/checkpatch.pl --strict. Anything that --strict notifies you of and that is reasonable to fix (not everything can be fixed) should be fixed. Also (although perhaps out of scope for a coding style) before new V4L2 drivers or substantial enhancements to V4L2 drivers can be accepted, you must run 'v4l2-compliance -s' for the video device (or even better use -m if the driver creates a media device) and include the output with the cover letter of the patch series. Obviously, any failures should be fixed. Regards, Hans > > Regards, > > Hans > >> + >> Key Cycle Dates >> --------------- >> >> -- >> 2.33.0 >> >
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 > > >
Em Thu, 21 Oct 2021 16:10:08 +0200 Hans Verkuil <hverkuil@xs4all.nl> escreveu: > There is one other requirement: the patches must be run through > scripts/checkpatch.pl --strict. Anything that --strict notifies you of and > that is reasonable to fix (not everything can be fixed) should be fixed. This is already there: Coding Style Addendum +++++++++++++++++++++ Media development uses ``checkpatch.pl`` on strict mode to verify the code style, e.g.:: $ ./scripts/checkpatch.pl --strict --max-line-length=80 > > Also (although perhaps out of scope for a coding style) before new V4L2 > drivers or substantial enhancements to V4L2 drivers can be accepted, you must > run 'v4l2-compliance -s' for the video device (or even better use -m if the driver > creates a media device) and include the output with the cover letter of > the patch series. Obviously, any failures should be fixed. This is also there: There is a set of compliance tools at https://git.linuxtv.org/v4l-utils.git/ that should be used in order to check if the drivers are properly implementing the media APIs: ==================== ======================================================= Type Tool ==================== ======================================================= V4L2 drivers\ [3]_ ``v4l2-compliance`` V4L2 virtual drivers ``contrib/test/test-media`` CEC drivers ``cec-compliance`` ==================== ======================================================= .. [3] The ``v4l2-compliance`` also covers the media controller usage inside V4L2 drivers. Other compilance tools are under development to check other parts of the subsystem. Those tests need to pass before the patches go upstream. Regards, Mauro
On Thu, Oct 21, 2021 at 04:00:40PM +0200, Hans Verkuil wrote: > 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 > > > + - 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; > > 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. This should probably go as the top-level, it's a valid comment for most (all ?) rules. > > + > > + 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; > > > + > > Key Cycle Dates > > --------------- > >
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 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; Cording style is one of the main candidate areas for bikeshedding. The first question that we should answer, I believe, is whether or not we want to define a more precise coding style for the subsystem to achieve higher uniformity, and how much latitude we want to give to developers. For instance, I don't mind unsigned int i, j; too much, but I would scream in horror at char *name = dev_name, c = '\0'; (I'm sad C even allows declaring a char pointer and a char variable on the same line like this). There are lots of cases between those two extremes that are more or less good (or bad) depending on who you ask, so we won't be able to come up with a precise set of rules that draw a line somewhere in the middle. What we could do is err more on the side of strictness, for instance with - One variable declaration per line. As an exception, grouping multiple single-letter counter variables on a single line is allowed. (or even allowing no exception). This is probably stricter than it needs to be, and in some cases it will result in a few more lines of code, but if it brings increased readability and maintainability through uniformity it's something we could consider. The same reasoning can apply to C++ comments, we can decide to allow them or not, but the more flexibility there will be in the rules, the less uniformity we'll have, which I personally believe hinders readability. Please don't reply to details of this specific example here, what I'd like to discuss first is the overall scope and principles. > > 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 > > > --------------- > > >
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 > > ---------------
Em Thu, 21 Oct 2021 18:31:15 +0300 Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu: > Hi Mauro, > > > > > + - 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; > > Cording style is one of the main candidate areas for bikeshedding. The > first question that we should answer, I believe, is whether or not we > want to define a more precise coding style for the subsystem to achieve > higher uniformity, and how much latitude we want to give to developers. I would prefer to give more freedom to developers, provided that the code is easy to read/maintain. Having to request multiple reviews just due coding style nitpicking seems to be a waste of time for everyone ;-) > For instance, I don't mind > > unsigned int i, j; > > too much, but I would scream in horror at > > char *name = dev_name, c = '\0'; Yeah, multiple vars being declared and assigned at the same line is something that should be avoided. See, even single letter vars with obvious assigns, like: int i = 0, j = 1; are less readable than: int i = 0; int j = 1; > (I'm sad C even allows declaring a char pointer and a char variable on > the same line like this). There are lots of cases between those two > extremes that are more or less good (or bad) depending on who you ask, > so we won't be able to come up with a precise set of rules that draw a > line somewhere in the middle. What we could do is err more on the side > of strictness, for instance with > > - One variable declaration per line. As an exception, grouping multiple > single-letter counter variables on a single line is allowed. > > (or even allowing no exception). This is probably stricter than it needs > to be, and in some cases it will result in a few more lines of code, but > if it brings increased readability and maintainability through > uniformity it's something we could consider. I don't think that things like: int ret, i, j; are less readable/maintainable than: int ret; int i; int j; Between the above, I would opt to the shorter format, when there's no variable initialization (no matter if the vars have single or multiple chars). On the other hand, I won't be nacking/rejecting a patch if it uses the longer format, as, for me, both are equivalent, in terms of maintenance and readability. So, for me, the rule should be just: - don't declare and initialize multiple variables at the same line. > > The same reasoning can apply to C++ comments, we can decide to allow > them or not, but the more flexibility there will be in the rules, the > less uniformity we'll have, which I personally believe hinders > readability. Yeah, agreed. Regards, Mauro
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 > > > > >
Hi Mauro, Laurent, On Thu, Oct 21, 2021 at 05:17:59PM +0100, Mauro Carvalho Chehab wrote: > Em Thu, 21 Oct 2021 18:31:15 +0300 > Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu: > > > Hi Mauro, > > > > > > > > + - 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; > > > > Cording style is one of the main candidate areas for bikeshedding. The > > first question that we should answer, I believe, is whether or not we > > want to define a more precise coding style for the subsystem to achieve > > higher uniformity, and how much latitude we want to give to developers. > > I would prefer to give more freedom to developers, provided that the > code is easy to read/maintain. Having to request multiple reviews just > due coding style nitpicking seems to be a waste of time for everyone ;-) > I agree in principle, but at the same time, a particularly stubborn confrontation during a review made me realize that most 'rules' are tribal knowledge, and a particularly stubborn developer might impose his own preferences arguing that everything that is not prohibited is allowed. If you add to that in the most common case cargo cult is the default way to find out what a rule is, if one driver escapes others will take inspiration from it. Now, I'm fine if it gets decided that everything not prohibited is allowed, but then I fear it will be very hard to maintain a consistent style among the subsystem. > > For instance, I don't mind > > > > unsigned int i, j; > > > > too much, but I would scream in horror at > > > > char *name = dev_name, c = '\0'; > > Yeah, multiple vars being declared and assigned at the same line is something > that should be avoided. See, even single letter vars with obvious assigns, > like: > > int i = 0, j = 1; > > are less readable than: > > int i = 0; > int j = 1; > > > (I'm sad C even allows declaring a char pointer and a char variable on > > the same line like this). There are lots of cases between those two > > extremes that are more or less good (or bad) depending on who you ask, > > so we won't be able to come up with a precise set of rules that draw a > > line somewhere in the middle. What we could do is err more on the side > > of strictness, for instance with > > > > - One variable declaration per line. As an exception, grouping multiple > > single-letter counter variables on a single line is allowed. > > > > (or even allowing no exception). This is probably stricter than it needs > > to be, and in some cases it will result in a few more lines of code, but > > if it brings increased readability and maintainability through > > uniformity it's something we could consider. > > I don't think that things like: > > int ret, i, j; > > are less readable/maintainable than: > > int ret; > int i; > int j; > > Between the above, I would opt to the shorter format, when there's no > variable initialization (no matter if the vars have single or multiple > chars). > > On the other hand, I won't be nacking/rejecting a patch if it uses > the longer format, as, for me, both are equivalent, in terms of > maintenance and readability. > > So, for me, the rule should be just: > > - don't declare and initialize multiple variables at the same line. > > > > > The same reasoning can apply to C++ comments, we can decide to allow > > them or not, but the more flexibility there will be in the rules, the > > less uniformity we'll have, which I personally believe hinders > > readability. > > Yeah, agreed. Thanks, I'll send a new version taking all your comments into account. Thanks j > > Regards, > Mauro
Hello, On Thu, Oct 21, 2021 at 08:20:42PM +0200, Jacopo Mondi wrote: > On Thu, Oct 21, 2021 at 05:17:59PM +0100, Mauro Carvalho Chehab wrote: > > Em Thu, 21 Oct 2021 18:31:15 +0300 Laurent Pinchart escreveu: > > > > > > > > + - 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; > > > > > > Cording style is one of the main candidate areas for bikeshedding. The > > > first question that we should answer, I believe, is whether or not we > > > want to define a more precise coding style for the subsystem to achieve > > > higher uniformity, and how much latitude we want to give to developers. > > > > I would prefer to give more freedom to developers, provided that the > > code is easy to read/maintain. Having to request multiple reviews just > > due coding style nitpicking seems to be a waste of time for everyone ;-) > > I agree in principle, but at the same time, a particularly stubborn > confrontation during a review made me realize that most 'rules' are > tribal knowledge, and a particularly stubborn developer might impose > his own preferences arguing that everything that is not prohibited is > allowed. If you add to that in the most common case cargo cult is > the default way to find out what a rule is, if one driver escapes > others will take inspiration from it. > > Now, I'm fine if it gets decided that everything not prohibited is > allowed, but then I fear it will be very hard to maintain a consistent > style among the subsystem. I agree with this. Possibly more problematic than a consistent style, we will then also have different reviewers asking for different style changes during review, which will be confusing for developers and will waste everybody's type. I see a more detailed style guide as a way to streamline the process and make it more efficient. > > > For instance, I don't mind > > > > > > unsigned int i, j; > > > > > > too much, but I would scream in horror at > > > > > > char *name = dev_name, c = '\0'; > > > > Yeah, multiple vars being declared and assigned at the same line is something > > that should be avoided. See, even single letter vars with obvious assigns, > > like: > > > > int i = 0, j = 1; > > > > are less readable than: > > > > int i = 0; > > int j = 1; > > > > > (I'm sad C even allows declaring a char pointer and a char variable on > > > the same line like this). There are lots of cases between those two > > > extremes that are more or less good (or bad) depending on who you ask, > > > so we won't be able to come up with a precise set of rules that draw a > > > line somewhere in the middle. What we could do is err more on the side > > > of strictness, for instance with > > > > > > - One variable declaration per line. As an exception, grouping multiple > > > single-letter counter variables on a single line is allowed. > > > > > > (or even allowing no exception). This is probably stricter than it needs > > > to be, and in some cases it will result in a few more lines of code, but > > > if it brings increased readability and maintainability through > > > uniformity it's something we could consider. > > > > I don't think that things like: > > > > int ret, i, j; > > > > are less readable/maintainable than: > > > > int ret; > > int i; > > int j; > > > > Between the above, I would opt to the shorter format, when there's no > > variable initialization (no matter if the vars have single or multiple > > chars). > > > > On the other hand, I won't be nacking/rejecting a patch if it uses > > the longer format, as, for me, both are equivalent, in terms of > > maintenance and readability. > > > > So, for me, the rule should be just: > > > > - don't declare and initialize multiple variables at the same line. > > > > > > > > The same reasoning can apply to C++ comments, we can decide to allow > > > them or not, but the more flexibility there will be in the rules, the > > > less uniformity we'll have, which I personally believe hinders > > > readability. > > > > Yeah, agreed. > > Thanks, I'll send a new version taking all your comments into account.
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) + Key Cycle Dates ---------------
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(+) -- 2.33.0