Re: [edk2] [edk2-CCodingStandardsSpecification PATCH 2/2] Source Files / Spacing / Multi-line func. calls: allow condensed arguments

[Laszlo Ersek]

On 08/12/17 12:13, Leif Lindholm wrote:
> On Fri, Aug 11, 2017 at 01:45:12PM -0700, Jordan Justen wrote:
>>> +Use this line breaking style especially if it saves a format string or 
>>> complex
>>> +argument from being split, or when commenting on individual arguments.
>>> +
>>> +# 5.2.2.4.2 The "condensed arguments" style
>>> +
>>> +For most function calls and function-like macro invocations, the "one 
>>> argument
>>> +per line" style uses up valuable vertical space without utilizing readily
>>> +available horizontal space. Such statements are permitted to condense the
>>> +arguments and the closing parenthesis (or parentheses), up to the allowed 
>>> line
>>> +length. The indentation requirements are identical to those of the "one
>>> +argument per line" style.
>>> +
>>> +```c
>>> +CopyMem (Destination, Source, SIZE_4KB);
>>> +
>>> +Status = gBS->AllocatePool (EfiBootServicesData, sizeof 
>>> (DRIVER_NAME_INSTANCE),
>>> +);
>>
>> I prefer that we just have one style, and just drop the requirement
>> that multiline param lists can only have one arg per line. I think it
>> is good to have the first param start on the next line in this case.
>>
>>   Status = gBS->AllocatePool (
>>   EfiBootServicesData, sizeof (DRIVER_NAME_INSTANCE),
>>   );
> 
> I like this one better than Laszlo's proposal, since it keeps the
> start of each line of parameters in the same column.
> 
>> This compacts things somewhat, but still keeps the parameter list
>> aligned horizontally on subsequent lines.
>>
>> Regarding the closing parens being on a separate line, I think we
>> should also remove that as a requirement.
> 
> I would also support that change.
> 
>> I think you might want to break out the multiple params per line and
>> closing parens change out to allow for separate discussion.
> 
> That would make sense.
> 
> Laszlo - many thanks to starting this discussion!

Thanks all for the feedback. Before I attempt to post version 2, I'd
like to see the "preference" wording sorted out (raised by Mike and
Jordan). I.e., whether the new style should be merely tolerated (but
still frowned upon) or if it should be a first class citizen.

(Stating the obvious,) if the new style were accepted into the CCS,
personally I would switch to it for all new code written by me (even if
the code were for MdePkg or MdeModulePkg), independently of the
preference assigned to the new style.

I would not require OvmfPkg / ArmVirtPkg contributors to follow my
preference -- NB I've been enforcing the one style we now have in the
CCS, for the packages I co-maintain, in spite of my dislike for that
style -- but I might suggest the alternative style to others if I
perceived the vertical space consumption very disturbing.

Thanks!
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [edk2-CCodingStandardsSpecification PATCH 2/2] Source Files / Spacing / Multi-line func. calls: allow condensed arguments

[Leif Lindholm]

On Fri, Aug 11, 2017 at 01:45:12PM -0700, Jordan Justen wrote:
> > +Use this line breaking style especially if it saves a format string or 
> > complex
> > +argument from being split, or when commenting on individual arguments.
> > +
> > +# 5.2.2.4.2 The "condensed arguments" style
> > +
> > +For most function calls and function-like macro invocations, the "one 
> > argument
> > +per line" style uses up valuable vertical space without utilizing readily
> > +available horizontal space. Such statements are permitted to condense the
> > +arguments and the closing parenthesis (or parentheses), up to the allowed 
> > line
> > +length. The indentation requirements are identical to those of the "one
> > +argument per line" style.
> > +
> > +```c
> > +CopyMem (Destination, Source, SIZE_4KB);
> > +
> > +Status = gBS->AllocatePool (EfiBootServicesData, sizeof 
> > (DRIVER_NAME_INSTANCE),
> > +);
> 
> I prefer that we just have one style, and just drop the requirement
> that multiline param lists can only have one arg per line. I think it
> is good to have the first param start on the next line in this case.
> 
>   Status = gBS->AllocatePool (
>   EfiBootServicesData, sizeof (DRIVER_NAME_INSTANCE),
>   );

I like this one better than Laszlo's proposal, since it keeps the
start of each line of parameters in the same column.

> This compacts things somewhat, but still keeps the parameter list
> aligned horizontally on subsequent lines.
> 
> Regarding the closing parens being on a separate line, I think we
> should also remove that as a requirement.

I would also support that change.

> I think you might want to break out the multiple params per line and
> closing parens change out to allow for separate discussion.

That would make sense.

Laszlo - many thanks to starting this discussion!

/
Leif
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [edk2-CCodingStandardsSpecification PATCH 2/2] Source Files / Spacing / Multi-line func. calls: allow condensed arguments

[Jordan Justen]

On 2017-08-11 14:04:21, Kinney, Michael D wrote:
> Laszlo,
> 
> I actually prefer the one arg per line in the call
> to match the function prototype that requires one
> arg per line.

One per line on prototypes seems reasonable.

When it come to calling, there are potentially many calls per function
prototype/definition.

The rule for calls is inconsistent regarding if one or more than one
line is used. If the parameters fit on one line, then it is okay to
have multiple params on a line. Why is it okay for the single line
case, if not for the multiple line case?

> Since most of the code follows this one arg per line
> convention today, I would not be in favor of a 
> non-backwards compatible change to the coding style.

I agree, which is why I suggest we let multiple params appear per
line, but if multiple lines are used, no params should appear on the
first line with the function name. Therefore the old code follows the
style. It simply breaks the params up onto more lines.

Regarding the separate line for ending the function call: ');' If we
make this optional, then the old code will also still follow the
style.

> Either leave it alone.  Or if there is a really strong
> desire to put multiple args in a single line then 
> document that as an optional supported style with the
> preferred style being one arg per line.

Would my suggestion be an ok compromise that would allow the old style
to still be used?

I don't know about stating that there is a preference for a single
line per param. Do we need to feel bad if we write or accept patches
that don't follow this style? :) If we state that the project has a
preference, then it doesn't seem like personal preference should be
enough of a reason to go against the project preference.

-Jordan

> 
> > -Original Message-
> > From: Justen, Jordan L
> > Sent: Friday, August 11, 2017 1:45 PM
> > To: Laszlo Ersek <ler...@redhat.com>; edk2-devel-01  > de...@lists.01.org>
> > Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Leif
> > Lindholm <leif.lindh...@linaro.org>; Ard Biesheuvel
> > <ard.biesheu...@linaro.org>
> > Subject: Re: [edk2] [edk2-CCodingStandardsSpecification PATCH
> > 2/2] Source Files / Spacing / Multi-line func. calls: allow
> > condensed arguments
> > 
> > On 2017-08-11 09:48:51, Laszlo Ersek wrote:
> > > The "one argument per line" style as the sole possibility
> > takes up too
> > > much vertical space, wastes perfectly good horizontal space,
> > and causes a
> > > constant jump-to-the-left eye movement for the reader.
> > >
> > > Now that we have limited the line length to 80 colums, the
> > "condensed
> > > arguments" style cannot be abused; permit it.
> > >
> > > Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>
> > > Cc: Jordan Justen <jordan.l.jus...@intel.com>
> > > Cc: Leif Lindholm <leif.lindh...@linaro.org>
> > > Cc: Michael D Kinney <michael.d.kin...@intel.com>
> > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > Signed-off-by: Laszlo Ersek <ler...@redhat.com>
> > > ---
> > >  5_source_files/52_spacing.md | 29 +++-
> > >  README.md|  1 +
> > >  2 files changed, 29 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/5_source_files/52_spacing.md
> > b/5_source_files/52_spacing.md
> > > index ddeabf7753a8..fc8a5e7e58e7 100644
> > > --- a/5_source_files/52_spacing.md
> > > +++ b/5_source_files/52_spacing.md
> > > @@ -133,7 +133,9 @@ If ((--MyInteger) > 0) {
> > >   5.2.2.4 Subsequent lines of multi-line function calls
> > should line up two spaces from the beginning of the function
> > name
> > >
> > >  If a function call or function like macro invocation is
> > broken up into multiple
> > > -lines, then:
> > > +lines, then follow one of the alternatives below.
> > > +
> > > +# 5.2.2.4.1 The "one argument per line" style
> > >
> > >  * One argument per line, including the first argument on its
> > own line.
> > >  * Indent each argument 2 spaces from the start of the
> > function name. If a
> > > @@ -165,6 +167,31 @@ DEBUG ((
> > >));
> > >  ```
> > >
> > > +Use this line breaking style especially if it saves a format
> > string or complex
> > > +argument from being split, or when commenting on individual
> > arguments.
> > > +
> > > +# 5.2.2.4.2 The "condensed arguments" style
> > > +
> > > +For most functi

Re: [edk2] [edk2-CCodingStandardsSpecification PATCH 2/2] Source Files / Spacing / Multi-line func. calls: allow condensed arguments

[Andrew Fish]

> On Aug 11, 2017, at 1:45 PM, Jordan Justen  wrote:
> 
> On 2017-08-11 09:48:51, Laszlo Ersek wrote:
>> The "one argument per line" style as the sole possibility takes up too
>> much vertical space, wastes perfectly good horizontal space, and causes a
>> constant jump-to-the-left eye movement for the reader.
>> 
>> Now that we have limited the line length to 80 colums, the "condensed
>> arguments" style cannot be abused; permit it.
>> 
>> Cc: Ard Biesheuvel 
>> Cc: Jordan Justen 
>> Cc: Leif Lindholm 
>> Cc: Michael D Kinney 
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Laszlo Ersek 
>> ---
>> 5_source_files/52_spacing.md | 29 +++-
>> README.md|  1 +
>> 2 files changed, 29 insertions(+), 1 deletion(-)
>> 
>> diff --git a/5_source_files/52_spacing.md b/5_source_files/52_spacing.md
>> index ddeabf7753a8..fc8a5e7e58e7 100644
>> --- a/5_source_files/52_spacing.md
>> +++ b/5_source_files/52_spacing.md
>> @@ -133,7 +133,9 @@ If ((--MyInteger) > 0) {
>>  5.2.2.4 Subsequent lines of multi-line function calls should line up 
>> two spaces from the beginning of the function name
>> 
>> If a function call or function like macro invocation is broken up into 
>> multiple
>> -lines, then:
>> +lines, then follow one of the alternatives below.
>> +
>> +# 5.2.2.4.1 The "one argument per line" style
>> 
>> * One argument per line, including the first argument on its own line.
>> * Indent each argument 2 spaces from the start of the function name. If a
>> @@ -165,6 +167,31 @@ DEBUG ((
>>   ));
>> ```
>> 
>> +Use this line breaking style especially if it saves a format string or 
>> complex
>> +argument from being split, or when commenting on individual arguments.
>> +
>> +# 5.2.2.4.2 The "condensed arguments" style
>> +
>> +For most function calls and function-like macro invocations, the "one 
>> argument
>> +per line" style uses up valuable vertical space without utilizing readily
>> +available horizontal space. Such statements are permitted to condense the
>> +arguments and the closing parenthesis (or parentheses), up to the allowed 
>> line
>> +length. The indentation requirements are identical to those of the "one
>> +argument per line" style.
>> +
>> +```c
>> +CopyMem (Destination, Source, SIZE_4KB);
>> +
>> +Status = gBS->AllocatePool (EfiBootServicesData, sizeof 
>> (DRIVER_NAME_INSTANCE),
>> +);
> 
> I prefer that we just have one style, and just drop the requirement
> that multiline param lists can only have one arg per line. I think it
> is good to have the first param start on the next line in this case.
> 
>  Status = gBS->AllocatePool (
>  EfiBootServicesData, sizeof (DRIVER_NAME_INSTANCE),
>  );
> 

I like using as much width as allowed, but then again I've got 3440 X 1440 
monitors I'm using all the time. 

Another place that multiple things on the same line make sense is 
InstallMultipleProtocolInterfaces. I think that makes the code more readable. 

Status - gBS->InstallMultipleProtocolInterfaces (
NULL,
,
, 
,
, 
NULL
);

Given a lot of this stuff is arbitrary (I had fun on the 1st coding standard 
doc) I think staying with what a lot of the code already does is not a bad idea.

Thanks,

Andrew Fish


> This compacts things somewhat, but still keeps the parameter list
> aligned horizontally on subsequent lines.
> 
> Regarding the closing parens being on a separate line, I think we
> should also remove that as a requirement.
> 
> I think you might want to break out the multiple params per line and
> closing parens change out to allow for separate discussion.
> 
> -Jordan
> 
>> +
>> +DEBUG ((DEBUG_INFO, "The addresses of the 4 buffers are %p, %p, %p, and 
>> %p\n",
>> +  Buffer1, Buffer2, Buffer3, Buffer4));
>> +```
>> +
>> +This line breaking style prevents overly frequent saccades to the left, 
>> without
>> +resulting in overlong lines.
>> +
>>  5.2.2.5 Always put space after commas or semicolons that separate items
>> 
>> This punctuation is not necessary if no code or comments follow the comma or
>> diff --git a/README.md b/README.md
>> index 8fad5a327b8c..437024e57677 100644
>> --- a/README.md
>> +++ b/README.md
>> @@ -113,3 +113,4 @@ Copyright (c) 2006-2017, Intel Corporation. All rights 
>> reserved.
>> | 2.2  | Convert to Gitbook  
>>  
>>  | June 2017  |
>> |  | [#425](https://bugzilla.tianocore.org/show_bug.cgi?id=425) 
>> [CCS] clarify line breaking and indentation requirements for multi-line 
>> function calls ||
>> |  | Limit lines to 80 columns 

Re: [edk2] [edk2-CCodingStandardsSpecification PATCH 2/2] Source Files / Spacing / Multi-line func. calls: allow condensed arguments

[Kinney, Michael D]

Laszlo,

I actually prefer the one arg per line in the call
to match the function prototype that requires one
arg per line.

Since most of the code follows this one arg per line
convention today, I would not be in favor of a 
non-backwards compatible change to the coding style.

Either leave it alone.  Or if there is a really strong
desire to put multiple args in a single line then 
document that as an optional supported style with the
preferred style being one arg per line.

Mike

> -Original Message-
> From: Justen, Jordan L
> Sent: Friday, August 11, 2017 1:45 PM
> To: Laszlo Ersek <ler...@redhat.com>; edk2-devel-01  de...@lists.01.org>
> Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Leif
> Lindholm <leif.lindh...@linaro.org>; Ard Biesheuvel
> <ard.biesheu...@linaro.org>
> Subject: Re: [edk2] [edk2-CCodingStandardsSpecification PATCH
> 2/2] Source Files / Spacing / Multi-line func. calls: allow
> condensed arguments
> 
> On 2017-08-11 09:48:51, Laszlo Ersek wrote:
> > The "one argument per line" style as the sole possibility
> takes up too
> > much vertical space, wastes perfectly good horizontal space,
> and causes a
> > constant jump-to-the-left eye movement for the reader.
> >
> > Now that we have limited the line length to 80 colums, the
> "condensed
> > arguments" style cannot be abused; permit it.
> >
> > Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>
> > Cc: Jordan Justen <jordan.l.jus...@intel.com>
> > Cc: Leif Lindholm <leif.lindh...@linaro.org>
> > Cc: Michael D Kinney <michael.d.kin...@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Laszlo Ersek <ler...@redhat.com>
> > ---
> >  5_source_files/52_spacing.md | 29 +++-
> >  README.md|  1 +
> >  2 files changed, 29 insertions(+), 1 deletion(-)
> >
> > diff --git a/5_source_files/52_spacing.md
> b/5_source_files/52_spacing.md
> > index ddeabf7753a8..fc8a5e7e58e7 100644
> > --- a/5_source_files/52_spacing.md
> > +++ b/5_source_files/52_spacing.md
> > @@ -133,7 +133,9 @@ If ((--MyInteger) > 0) {
> >   5.2.2.4 Subsequent lines of multi-line function calls
> should line up two spaces from the beginning of the function
> name
> >
> >  If a function call or function like macro invocation is
> broken up into multiple
> > -lines, then:
> > +lines, then follow one of the alternatives below.
> > +
> > +# 5.2.2.4.1 The "one argument per line" style
> >
> >  * One argument per line, including the first argument on its
> own line.
> >  * Indent each argument 2 spaces from the start of the
> function name. If a
> > @@ -165,6 +167,31 @@ DEBUG ((
> >));
> >  ```
> >
> > +Use this line breaking style especially if it saves a format
> string or complex
> > +argument from being split, or when commenting on individual
> arguments.
> > +
> > +# 5.2.2.4.2 The "condensed arguments" style
> > +
> > +For most function calls and function-like macro invocations,
> the "one argument
> > +per line" style uses up valuable vertical space without
> utilizing readily
> > +available horizontal space. Such statements are permitted to
> condense the
> > +arguments and the closing parenthesis (or parentheses), up to
> the allowed line
> > +length. The indentation requirements are identical to those
> of the "one
> > +argument per line" style.
> > +
> > +```c
> > +CopyMem (Destination, Source, SIZE_4KB);
> > +
> > +Status = gBS->AllocatePool (EfiBootServicesData, sizeof
> (DRIVER_NAME_INSTANCE),
> > +);
> 
> I prefer that we just have one style, and just drop the
> requirement
> that multiline param lists can only have one arg per line. I
> think it
> is good to have the first param start on the next line in this
> case.
> 
>   Status = gBS->AllocatePool (
>   EfiBootServicesData, sizeof
> (DRIVER_NAME_INSTANCE),
>   );
> 
> This compacts things somewhat, but still keeps the parameter
> list
> aligned horizontally on subsequent lines.
> 
> Regarding the closing parens being on a separate line, I think
> we
> should also remove that as a requirement.
> 
> I think you might want to break out the multiple params per line
> and
> closing parens change out to allow for separate discussion.
> 
> -Jordan
> 
> > +
> > +DEBUG ((DEBUG_INFO, "The addresses of the 4 buffers are %p,
> %p, %p, and %p\n",
> >

Re: [edk2] [edk2-CCodingStandardsSpecification PATCH 2/2] Source Files / Spacing / Multi-line func. calls: allow condensed arguments

[Jordan Justen]

On 2017-08-11 09:48:51, Laszlo Ersek wrote:
> The "one argument per line" style as the sole possibility takes up too
> much vertical space, wastes perfectly good horizontal space, and causes a
> constant jump-to-the-left eye movement for the reader.
> 
> Now that we have limited the line length to 80 colums, the "condensed
> arguments" style cannot be abused; permit it.
> 
> Cc: Ard Biesheuvel 
> Cc: Jordan Justen 
> Cc: Leif Lindholm 
> Cc: Michael D Kinney 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek 
> ---
>  5_source_files/52_spacing.md | 29 +++-
>  README.md|  1 +
>  2 files changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/5_source_files/52_spacing.md b/5_source_files/52_spacing.md
> index ddeabf7753a8..fc8a5e7e58e7 100644
> --- a/5_source_files/52_spacing.md
> +++ b/5_source_files/52_spacing.md
> @@ -133,7 +133,9 @@ If ((--MyInteger) > 0) {
>   5.2.2.4 Subsequent lines of multi-line function calls should line up 
> two spaces from the beginning of the function name
>  
>  If a function call or function like macro invocation is broken up into 
> multiple
> -lines, then:
> +lines, then follow one of the alternatives below.
> +
> +# 5.2.2.4.1 The "one argument per line" style
>  
>  * One argument per line, including the first argument on its own line.
>  * Indent each argument 2 spaces from the start of the function name. If a
> @@ -165,6 +167,31 @@ DEBUG ((
>));
>  ```
>  
> +Use this line breaking style especially if it saves a format string or 
> complex
> +argument from being split, or when commenting on individual arguments.
> +
> +# 5.2.2.4.2 The "condensed arguments" style
> +
> +For most function calls and function-like macro invocations, the "one 
> argument
> +per line" style uses up valuable vertical space without utilizing readily
> +available horizontal space. Such statements are permitted to condense the
> +arguments and the closing parenthesis (or parentheses), up to the allowed 
> line
> +length. The indentation requirements are identical to those of the "one
> +argument per line" style.
> +
> +```c
> +CopyMem (Destination, Source, SIZE_4KB);
> +
> +Status = gBS->AllocatePool (EfiBootServicesData, sizeof 
> (DRIVER_NAME_INSTANCE),
> +);

I prefer that we just have one style, and just drop the requirement
that multiline param lists can only have one arg per line. I think it
is good to have the first param start on the next line in this case.

  Status = gBS->AllocatePool (
  EfiBootServicesData, sizeof (DRIVER_NAME_INSTANCE),
  );

This compacts things somewhat, but still keeps the parameter list
aligned horizontally on subsequent lines.

Regarding the closing parens being on a separate line, I think we
should also remove that as a requirement.

I think you might want to break out the multiple params per line and
closing parens change out to allow for separate discussion.

-Jordan

> +
> +DEBUG ((DEBUG_INFO, "The addresses of the 4 buffers are %p, %p, %p, and 
> %p\n",
> +  Buffer1, Buffer2, Buffer3, Buffer4));
> +```
> +
> +This line breaking style prevents overly frequent saccades to the left, 
> without
> +resulting in overlong lines.
> +
>   5.2.2.5 Always put space after commas or semicolons that separate items
>  
>  This punctuation is not necessary if no code or comments follow the comma or
> diff --git a/README.md b/README.md
> index 8fad5a327b8c..437024e57677 100644
> --- a/README.md
> +++ b/README.md
> @@ -113,3 +113,4 @@ Copyright (c) 2006-2017, Intel Corporation. All rights 
> reserved.
>  | 2.2  | Convert to Gitbook  
>   
> | June 2017  |
>  |  | [#425](https://bugzilla.tianocore.org/show_bug.cgi?id=425) 
> [CCS] clarify line breaking and indentation requirements for multi-line 
> function calls ||
>  |  | Limit lines to 80 columns   
>   
> ||
> +|  | Introduce the "condensed arguments" line breaking style 
>   
> ||
> -- 
> 2.13.1.3.g8be5a757fa67
> 
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel