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

[Laszlo Ersek <lersek@redhat.com>]

On 03/14/17 02:58, Kinney, Michael D wrote:
> 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.

If I understand correctly, that means v2 of this set should be acceptable.

If that's right, can you please R-b this set, Jordan?

> I would prefer everyone follow
> this specific recommendation if possible.

I can do that, in the future, if we codify the preference in some
written form, even before the CCS is updated and/or imported to
gitbooks. Should we file a TianoCore BZ for it? I can do that, if you
prefer.

Thanks!
Laszlo

> 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