Re: [PATCH 11/12] OvmfPkg/SmmControl2Dxe: save fw_cfg boot script with QemuFwCfgS3Lib

["Kinney, Michael D" <michael.d.kinney@intel.com>]

Laszlo,

I agree that there are some inconsistencies in the edk2 C source files.

For this specific example, I prefer what Jordan recommended:

> >   Status = QemuFwCfgS3WriteBytes (
> >              mRequestedFeaturesItem,
> >              sizeof ScratchBuffer->Features
> >              );

When multi-line:
* One argument per line
* Indent each argument 2 spaces from start of function name
* Close parenthesis aligned with start of arguments

I am actively working on adding the EDK II C Coding standard
and other EDK II docs to gitbooks.  That will be a good place
to discuss and clarify or correct the EDK II C coding standard.

Given inconsistencies in some of the edk2 C source files and 
some clarification needed in EDK II C Coding Standard, I would
not require any changes here.  I would prefer everyone follow
this specific recommendation if possible.  

Hopefully we can expand the PatchCheck tool over time to help
check code against the EDK II C Coding Standard.

Mike


> -----Original Message-----
> From: Justen, Jordan L
> Sent: Monday, March 13, 2017 3:44 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Cc: edk2-devel-01 <edk2-devel@lists.01.org>
> Subject: Re: [edk2] [PATCH 11/12] OvmfPkg/SmmControl2Dxe: save fw_cfg boot script with
> QemuFwCfgS3Lib
> 
> On 2017-03-13 15:12:08, Laszlo Ersek wrote:
> >
> > "CCS_2_1_Draft.pdf" seems to be the most recent version; in it, I find:
> >
> > (1)
> >
> > 5.2.2.4 Subsequent lines of multi-line function calls should line up
> >         one or two tab-stops from the beginning of the function name
> >
> >         Use either one or two tab stops to ensure that each parameter
> >         is indented at least two spaces after the function name. Either
> >         of the below examples is acceptable:
> >
> >         Status = gBS->AllocatePool (
> >                         EfiBootServicesData,
> >                         sizeof (DRIVER_NAME_INSTANCE),
> >                         &PrivateData
> >                       );
> >
> >        Status = gBS->AllocatePool (
> >                        EfiBootServicesData,
> >                        sizeof (DRIVER_NAME_INSTANCE),
> >                        &PrivateData
> >                      );
> >
> > My notes on this:
> >
> > - my code complies with the indentation requirement
> >
> > - the passage is silent on whether each argument should be broken off
> >   to a new line. The examples are called "acceptable", not "required".
> >   I guess this is what your question to Mike is about.
> >
> 
> Right. It seems like this is a case where the text perhaps could be
> more clear. Just looking at the precedence in the tree, it seems
> likely that we might want to say that if the call requires multiple
> lines, then the first parameter should be indented on the next line.
> 
> > - I don't see what the difference is between the two examples. To me
> >   they look identical.
> 
> I think this example formatting got messed up. I saw a previous
> version that varied very subtlely.
> 
>         Status = gBS->AllocatePool (
>                         EfiBootServicesData,
>                         sizeof (DRIVER_NAME_INSTANCE),
>                         &PrivateData
>                       );
> 
>         Status =  gBS->AllocatePool (
>                           EfiBootServicesData,
>                           sizeof (DRIVER_NAME_INSTANCE),
>                           &PrivateData
>                        );
> 
> I always thought the rule was 2 spaces indenting beyond the function
> name.
> 
> These example make me think that the intension is that your tab key
> might only want to align to columns that are even numbered, so you
> might have the result be that the indent from the function name would
> be 3 spaces.
> 
> I don't know... I prefer the rule that you indent 2 spaces from the
> function name. :)
> 
> > - In the examples, the closing parens line up with "AllocatePool", not
> >   with "EfiBootServicesData". I think I have never ever seen this style
> >   in edk2.
> >
> 
> Yeah. Me either. This is confusing.
> 
> -Jordan