Re: [Outreachy kernel] [PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line
On Fri, Apr 16, 2021 at 11:37:28AM +0300, Sakari Ailus wrote: > Hi Dan, > > On Fri, Apr 16, 2021 at 08:49:41AM +0300, Dan Carpenter wrote: > > On Fri, Apr 16, 2021 at 12:21:58AM +0300, Sakari Ailus wrote: > > > On Thu, Apr 15, 2021 at 08:59:41PM +0100, Matthew Wilcox wrote: > > > > On Thu, Apr 15, 2021 at 08:57:04PM +0100, Matthew Wilcox wrote: > > > > > On Thu, Apr 15, 2021 at 10:49:55PM +0300, Sakari Ailus wrote: > > > > > > On Thu, Apr 15, 2021 at 06:14:09PM +0100, Matthew Wilcox wrote: > > > > > > > On Thu, Apr 15, 2021 at 02:08:19PM -0300, Aline Santana Cordeiro > > > > > > > wrote: > > > > > > > > -const struct atomisp_format_bridge > > > > > > > > *get_atomisp_format_bridge_from_mbus( > > > > > > > > -u32 mbus_code); > > > > > > > > +const struct atomisp_format_bridge* > > > > > > > > +get_atomisp_format_bridge_from_mbus(u32 mbus_code); > > > > > > > > > > > > > > No, this does not match coding style. Probably best to break the > > > > > > > 80-column guideline in this instance. Best would be to have a > > > > > > > function > > > > > > > > > > > > Having the return type on the previous line is perfectly fine. > > > > > > There should > > > > > > be a space before the asterisk though. > > > > > > > > > > No, it's not. Linus has ranted about that before. > > > > > > > > Found it. > > > > https://lore.kernel.org/lkml/1054519757.161...@palladium.transmeta.com/ > > > > > > Two decades ago, really? > > > > > > This is simply one of the practical means how you split long function > > > declarations and avoid overly long lines. Not my favourite though, but > > > still better than those long lines. > > > > I've always thought we allow either style, but it has to be done > > consistently within the file. I was pretty sure that was policy but > > it's another thing that goes back decades so I don't have a reference. > > It shouldn't be about breaking up long lines. > > > > > > > > My personal preference would be to wrap at the opening parenthesis and > > > indent by just a tab, but I know many people who disagree with that... > > > > If you're running into the 80 character limit, then it's fine to use > > two tabs. I think we have been rejecting patches that push align the > > parameters but push past the 80 character limit. Using one tab is > > confusing because it makes the decalarations line up with the code. > > Interesting. Do you have an example of this? I've thought checkpatch.pl > gave a warning if the line ended with an opening parenthesis no matter > what. The prefered style is still aligning with the parentheses but if you have to choose between a warning about going over the limit or a warning about aligning then probably it's fine to not align. I can't find an example, but I'm pretty sure we've been rejecting patches that align the parameters but now go over the 80/100 char limit. regards, dan carpenter
Re: [Outreachy kernel] [PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line
On Fri, 16 Apr 2021, Sakari Ailus wrote: > On Fri, Apr 16, 2021 at 10:46:54AM +0200, Julia Lawall wrote: > > > > If you're running into the 80 character limit, then it's fine to use > > > > two tabs. I think we have been rejecting patches that push align the > > > > parameters but push past the 80 character limit. Using one tab is > > > > confusing because it makes the decalarations line up with the code. > > > > > > Interesting. Do you have an example of this? I've thought checkpatch.pl > > > gave a warning if the line ended with an opening parenthesis no matter > > > what. > > > > Checkpatch is a perl script that searches for patterns that often indicate > > code that is hard to understand. It is not a precise definition of what > > is allowed in the Linux kernel. Humans have to amke compromises. > > Yeah... but I'd think if this is a preferred style then the warning could > be omitted. It might not be that difficult. No idea. It involves looking at two successive lines, which may make it more complicated. Probably the biggest problem would be knowing whether the line being looked at represents a function header. Maybe that could be detected by the fact that there is normally no space at the beginning of the line? julia
Re: [Outreachy kernel] [PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line
On Fri, Apr 16, 2021 at 10:46:54AM +0200, Julia Lawall wrote: > > > If you're running into the 80 character limit, then it's fine to use > > > two tabs. I think we have been rejecting patches that push align the > > > parameters but push past the 80 character limit. Using one tab is > > > confusing because it makes the decalarations line up with the code. > > > > Interesting. Do you have an example of this? I've thought checkpatch.pl > > gave a warning if the line ended with an opening parenthesis no matter > > what. > > Checkpatch is a perl script that searches for patterns that often indicate > code that is hard to understand. It is not a precise definition of what > is allowed in the Linux kernel. Humans have to amke compromises. Yeah... but I'd think if this is a preferred style then the warning could be omitted. It might not be that difficult. -- Sakari Ailus
Re: [Outreachy kernel] [PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line
> > If you're running into the 80 character limit, then it's fine to use > > two tabs. I think we have been rejecting patches that push align the > > parameters but push past the 80 character limit. Using one tab is > > confusing because it makes the decalarations line up with the code. > > Interesting. Do you have an example of this? I've thought checkpatch.pl > gave a warning if the line ended with an opening parenthesis no matter > what. Checkpatch is a perl script that searches for patterns that often indicate code that is hard to understand. It is not a precise definition of what is allowed in the Linux kernel. Humans have to amke compromises. julia
Re: [Outreachy kernel] [PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line
Hi Dan, On Fri, Apr 16, 2021 at 08:49:41AM +0300, Dan Carpenter wrote: > On Fri, Apr 16, 2021 at 12:21:58AM +0300, Sakari Ailus wrote: > > On Thu, Apr 15, 2021 at 08:59:41PM +0100, Matthew Wilcox wrote: > > > On Thu, Apr 15, 2021 at 08:57:04PM +0100, Matthew Wilcox wrote: > > > > On Thu, Apr 15, 2021 at 10:49:55PM +0300, Sakari Ailus wrote: > > > > > On Thu, Apr 15, 2021 at 06:14:09PM +0100, Matthew Wilcox wrote: > > > > > > On Thu, Apr 15, 2021 at 02:08:19PM -0300, Aline Santana Cordeiro > > > > > > wrote: > > > > > > > -const struct atomisp_format_bridge > > > > > > > *get_atomisp_format_bridge_from_mbus( > > > > > > > -u32 mbus_code); > > > > > > > +const struct atomisp_format_bridge* > > > > > > > +get_atomisp_format_bridge_from_mbus(u32 mbus_code); > > > > > > > > > > > > No, this does not match coding style. Probably best to break the > > > > > > 80-column guideline in this instance. Best would be to have a > > > > > > function > > > > > > > > > > Having the return type on the previous line is perfectly fine. There > > > > > should > > > > > be a space before the asterisk though. > > > > > > > > No, it's not. Linus has ranted about that before. > > > > > > Found it. > > > https://lore.kernel.org/lkml/1054519757.161...@palladium.transmeta.com/ > > > > Two decades ago, really? > > > > This is simply one of the practical means how you split long function > > declarations and avoid overly long lines. Not my favourite though, but > > still better than those long lines. > > I've always thought we allow either style, but it has to be done > consistently within the file. I was pretty sure that was policy but > it's another thing that goes back decades so I don't have a reference. > It shouldn't be about breaking up long lines. > > > > > My personal preference would be to wrap at the opening parenthesis and > > indent by just a tab, but I know many people who disagree with that... > > If you're running into the 80 character limit, then it's fine to use > two tabs. I think we have been rejecting patches that push align the > parameters but push past the 80 character limit. Using one tab is > confusing because it makes the decalarations line up with the code. Interesting. Do you have an example of this? I've thought checkpatch.pl gave a warning if the line ended with an opening parenthesis no matter what. -- Kind regards, Sakari Ailus
Re: [Outreachy kernel] [PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line
On Fri, Apr 16, 2021 at 12:21:58AM +0300, Sakari Ailus wrote: > On Thu, Apr 15, 2021 at 08:59:41PM +0100, Matthew Wilcox wrote: > > On Thu, Apr 15, 2021 at 08:57:04PM +0100, Matthew Wilcox wrote: > > > On Thu, Apr 15, 2021 at 10:49:55PM +0300, Sakari Ailus wrote: > > > > On Thu, Apr 15, 2021 at 06:14:09PM +0100, Matthew Wilcox wrote: > > > > > On Thu, Apr 15, 2021 at 02:08:19PM -0300, Aline Santana Cordeiro > > > > > wrote: > > > > > > -const struct atomisp_format_bridge > > > > > > *get_atomisp_format_bridge_from_mbus( > > > > > > -u32 mbus_code); > > > > > > +const struct atomisp_format_bridge* > > > > > > +get_atomisp_format_bridge_from_mbus(u32 mbus_code); > > > > > > > > > > No, this does not match coding style. Probably best to break the > > > > > 80-column guideline in this instance. Best would be to have a > > > > > function > > > > > > > > Having the return type on the previous line is perfectly fine. There > > > > should > > > > be a space before the asterisk though. > > > > > > No, it's not. Linus has ranted about that before. > > > > Found it. > > https://lore.kernel.org/lkml/1054519757.161...@palladium.transmeta.com/ > > Two decades ago, really? > > This is simply one of the practical means how you split long function > declarations and avoid overly long lines. Not my favourite though, but > still better than those long lines. I've always thought we allow either style, but it has to be done consistently within the file. I was pretty sure that was policy but it's another thing that goes back decades so I don't have a reference. It shouldn't be about breaking up long lines. > > My personal preference would be to wrap at the opening parenthesis and > indent by just a tab, but I know many people who disagree with that... If you're running into the 80 character limit, then it's fine to use two tabs. I think we have been rejecting patches that push align the parameters but push past the 80 character limit. Using one tab is confusing because it makes the decalarations line up with the code. regards, dan carpenter
Re: [Outreachy kernel] [PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line
On Thu, Apr 15, 2021 at 08:59:41PM +0100, Matthew Wilcox wrote: > On Thu, Apr 15, 2021 at 08:57:04PM +0100, Matthew Wilcox wrote: > > On Thu, Apr 15, 2021 at 10:49:55PM +0300, Sakari Ailus wrote: > > > On Thu, Apr 15, 2021 at 06:14:09PM +0100, Matthew Wilcox wrote: > > > > On Thu, Apr 15, 2021 at 02:08:19PM -0300, Aline Santana Cordeiro wrote: > > > > > -const struct atomisp_format_bridge > > > > > *get_atomisp_format_bridge_from_mbus( > > > > > -u32 mbus_code); > > > > > +const struct atomisp_format_bridge* > > > > > +get_atomisp_format_bridge_from_mbus(u32 mbus_code); > > > > > > > > No, this does not match coding style. Probably best to break the > > > > 80-column guideline in this instance. Best would be to have a function > > > > > > Having the return type on the previous line is perfectly fine. There > > > should > > > be a space before the asterisk though. > > > > No, it's not. Linus has ranted about that before. > > Found it. > https://lore.kernel.org/lkml/1054519757.161...@palladium.transmeta.com/ Two decades ago, really? This is simply one of the practical means how you split long function declarations and avoid overly long lines. Not my favourite though, but still better than those long lines. My personal preference would be to wrap at the opening parenthesis and indent by just a tab, but I know many people who disagree with that... -- Sakari Ailus
Re: [Outreachy kernel] [PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line
On Thu, Apr 15, 2021 at 08:57:04PM +0100, Matthew Wilcox wrote: > On Thu, Apr 15, 2021 at 10:49:55PM +0300, Sakari Ailus wrote: > > On Thu, Apr 15, 2021 at 06:14:09PM +0100, Matthew Wilcox wrote: > > > On Thu, Apr 15, 2021 at 02:08:19PM -0300, Aline Santana Cordeiro wrote: > > > > -const struct atomisp_format_bridge > > > > *get_atomisp_format_bridge_from_mbus( > > > > -u32 mbus_code); > > > > +const struct atomisp_format_bridge* > > > > +get_atomisp_format_bridge_from_mbus(u32 mbus_code); > > > > > > No, this does not match coding style. Probably best to break the > > > 80-column guideline in this instance. Best would be to have a function > > > > Having the return type on the previous line is perfectly fine. There should > > be a space before the asterisk though. > > No, it's not. Linus has ranted about that before. Found it. https://lore.kernel.org/lkml/1054519757.161...@palladium.transmeta.com/
Re: [Outreachy kernel] [PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line
On Thu, Apr 15, 2021 at 10:49:55PM +0300, Sakari Ailus wrote: > On Thu, Apr 15, 2021 at 06:14:09PM +0100, Matthew Wilcox wrote: > > On Thu, Apr 15, 2021 at 02:08:19PM -0300, Aline Santana Cordeiro wrote: > > > -const struct atomisp_format_bridge *get_atomisp_format_bridge_from_mbus( > > > -u32 mbus_code); > > > +const struct atomisp_format_bridge* > > > +get_atomisp_format_bridge_from_mbus(u32 mbus_code); > > > > No, this does not match coding style. Probably best to break the > > 80-column guideline in this instance. Best would be to have a function > > Having the return type on the previous line is perfectly fine. There should > be a space before the asterisk though. No, it's not. Linus has ranted about that before.
Re: [Outreachy kernel] [PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line
On Thu, Apr 15, 2021 at 06:14:09PM +0100, Matthew Wilcox wrote: > On Thu, Apr 15, 2021 at 02:08:19PM -0300, Aline Santana Cordeiro wrote: > > -const struct atomisp_format_bridge *get_atomisp_format_bridge_from_mbus( > > -u32 mbus_code); > > +const struct atomisp_format_bridge* > > +get_atomisp_format_bridge_from_mbus(u32 mbus_code); > > No, this does not match coding style. Probably best to break the > 80-column guideline in this instance. Best would be to have a function Having the return type on the previous line is perfectly fine. There should be a space before the asterisk though. -- Sakari Ailus
Re: [Outreachy kernel] [PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line
Em qui, 2021-04-15 às 18:14 +0100, Matthew Wilcox escreveu: > On Thu, Apr 15, 2021 at 02:08:19PM -0300, Aline Santana Cordeiro > wrote: > > -const struct atomisp_format_bridge > > *get_atomisp_format_bridge_from_mbus( > > - u32 mbus_code); > > +const struct atomisp_format_bridge* > > +get_atomisp_format_bridge_from_mbus(u32 mbus_code); > > No, this does not match coding style. Probably best to break the > 80-column guideline in this instance. Best would be to have a > function > and/or struct name that isn't so ridiculously long, but that would > require some in-depth thinking. > I left the type of function and its name with the parameters in different lines, following up some examples of other files, such as atomisp_acc.c. But I didn't pay attention and left the pointer with the function name instead of left it with the type of the function in v1, so Hans suggested it to a v2, as I did. What should I do in this case? Thank you in advance, Aline > > -void atomisp_apply_css_parameters( > > - struct atomisp_sub_device *asd, > > - struct atomisp_css_params *css_param); > > +void atomisp_apply_css_parameters(struct atomisp_sub_device *asd, > > + struct atomisp_css_params > > *css_param); > > + > > Good. >
Re: [Outreachy kernel] [PATCH v2] staging: media: atomisp: pci: Change line break to avoid an open parenthesis at the end of the line
On Thu, Apr 15, 2021 at 02:08:19PM -0300, Aline Santana Cordeiro wrote: > -const struct atomisp_format_bridge *get_atomisp_format_bridge_from_mbus( > -u32 mbus_code); > +const struct atomisp_format_bridge* > +get_atomisp_format_bridge_from_mbus(u32 mbus_code); No, this does not match coding style. Probably best to break the 80-column guideline in this instance. Best would be to have a function and/or struct name that isn't so ridiculously long, but that would require some in-depth thinking. > -void atomisp_apply_css_parameters( > -struct atomisp_sub_device *asd, > -struct atomisp_css_params *css_param); > +void atomisp_apply_css_parameters(struct atomisp_sub_device *asd, > + struct atomisp_css_params *css_param); > + Good.
