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

["Sean" <spbrogan@outlook.com>]

If you look through the bugs you will see there is some debate. 
Personally, I think there is never a single right answer on style and 
there will always be differing opinions.  The underlying goal that most 
agree on is providing consistency for a project and to make it easy for 
someone to contribute code that aligns with that consistent standard.   
Since 2017, a lot has changed but the biggest thing is we have a 
relatively easy, well documented, industry standard tool that supports 
style/formatting of edk2. These tools can be run locally by those 
contributing and are run by the CI system on pull request to enforce 
this consistency.  We have already reformatted the entire code base of 
edk2 to align with that and although i am not opposed to auto 
reformatting in the future for a great reason I don't see anything is 
these proposed changes that would justify that.


Thanks

Sean



On 11/14/2022 10:49 AM, Michael Kubacki wrote:
> I saw those, but they're from over 5 years ago and the community has 
> changed since then.
>
> This is an impactful change. It will impact every line of code written.
>
> I'm not suggesting this go to the Tools & CI Meeting because of 
> uncrustify. I'm not concerned with any impact to uncrustify.
>
> This came to my attention because I was added to thread. I would like 
> to suggest that we raise more awareness about this change in a meeting 
> with members of the community to capture additional feedback.
>
> Also, the logistics (uncrustify aside) about how to execute this 
> change are unclear (as written in this thread and the BZ) and it would 
> be easier/faster to discuss in a community call.
>
> Is that possible?
>
> Thanks,
> Michael
>
> On 11/14/2022 1:25 PM, Kinney, Michael D wrote:
>> Hi Michael,
>>
>> Yes.  They have been discussed.  There were patches and discussion on 
>> mailing
>> lists back in 2017.  Long before enabling uncrustify. I provided 
>> links to
>> these conversations in the following email about a week ago.
>>
>> https://edk2.groups.io/g/devel/message/96038
>>
>>
>> Mike
>>
>>> -----Original Message-----
>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of 
>>> Michael Kubacki
>>> Sent: Monday, November 14, 2022 10:05 AM
>>> To: devel@edk2.groups.io; Kinney, Michael D 
>>> <michael.d.kinney@intel.com>; abner.chang@amd.com; Laszlo Ersek 
>>> <lersek@redhat.com>;
>>> Kubacki, Michael <michael.kubacki@microsoft.com>
>>> Subject: Re: [edk2-devel] [edk2-CCodingStandardsSpecification PATCH 
>>> 2/2] Source Files / Spacing / Multi-line func. calls: allow
>>> condensed arguments
>>>
>>> Have these changes been discussed in a community forum? Do you think we
>>> could talk about it in the Tianocore Tools & CI Meeting today?
>>>
>>> Thanks,
>>> Michael
>>>
>>> On 11/14/2022 12:37 PM, Michael Kubacki wrote:
>>>> I'm catching up on this thread so let me know if I miss something.
>>>>
>>>> Uncrustify can perform conversions/enforcements like adjusting code 
>>>> to <
>>>> 80 columns.
>>>>
>>>> The edk2 uncrustify configuration file is here and you will see that I
>>>> commented column width enforcement:
>>>>
>>>> https://github.com/tianocore/edk2/blob/master/.pytool/Plugin/UncrustifyCheck/uncrustify.cfg#L19. 
>>>>
>>>>
>>>>
>>>> Uncrustify aside, column width is particularly difficult to adjust
>>>> consistently. Both humans and tools have to make many (constant)
>>>> situational decisions based on code structure.
>>>>
>>>> However, it should be possible. I've taken the current uncrustify
>>>> configuration file, made minimal changes to restrict code to 80 
>>>> columns,
>>>> and published the results based on the current edk2 code tree here:
>>>>
>>>> https://github.com/makubacki/edk2/tree/uncrustify_80_column_change
>>>>
>>>> You can see the I ran uncrustify 3 times to reach a mostly steady 
>>>> state.
>>>> The issue after the second run is that uncrustify is confused about 
>>>> what
>>>> to do with single macros that exceed 80 columns.
>>>>
>>>> Examples:
>>>> 1.
>>>> https://github.com/makubacki/edk2/blob/uncrustify_80_column_change/MdePkg/Include/IndustryStandard/Acpi30.h#L702-#L704 
>>>>
>>>>
>>>> 2.
>>>> https://github.com/makubacki/edk2/blob/uncrustify_80_column_change/MdePkg/Include/IndustryStandard/IpmiNetFnApp.h#L1023-#L1031 
>>>>
>>>>
>>>>
>>>> There are other cases there as well.
>>>>
>>>> Anyway, if those were reduced in length, I think we could reach a 
>>>> steady
>>>> state. Some other minor tweaks might also be required.
>>>>
>>>> Regarding function calls, I put together the following branch to
>>>> demonstrate some examples of what is done now.
>>>>
>>>> In summary, multiple arguments can be provided on a single line 
>>>> (with no
>>>> width or argument count maximum) or multiple lines. If a single 
>>>> argument
>>>> is multi-line, then all arguments must be on a unique line to 
>>>> follow the
>>>> multi-line argument convention.
>>>>
>>>> See the top two commits in this branch for examples:
>>>>
>>>> https://github.com/makubacki/edk2/tree/func_arg_formatting_demo
>>>>
>>>> I agree uncrustify and the spec be in sync.
>>>>
>>>> Regards,
>>>> Michael
>>>>
>>>> On 11/14/2022 12:07 PM, Michael D Kinney wrote:
>>>>> I disagree that they can coexist.
>>>>>
>>>>> If uncrustify is forcing 1 arg per line, then a developer that 
>>>>> follows
>>>>> a CSS that allows multiple per line, the code change will be rejected
>>>>> by EDK II CI.
>>>>>
>>>>> The CSS and Uncristify behavior need to be aligned.If we want a CSS
>>>>> change that requires Uncristify changes, then they have to be
>>>>> coordinated.
>>>>>
>>>>> Mike
>>>>>
>>>>> *From:*devel@edk2.groups.io <devel@edk2.groups.io> *On Behalf Of
>>>>> *Chang, Abner via groups.io
>>>>> *Sent:* Sunday, November 13, 2022 5:10 PM
>>>>> *To:* devel@edk2.groups.io; Kinney, Michael D
>>>>> <michael.d.kinney@intel.com>; Laszlo Ersek <lersek@redhat.com>;
>>>>> Kubacki, Michael <michael.kubacki@microsoft.com>
>>>>> *Subject:* Re: [edk2-devel] [edk2-CCodingStandardsSpecification PATCH
>>>>> 2/2] Source Files / Spacing / Multi-line func. calls: allow condensed
>>>>> arguments
>>>>>
>>>>> [AMD Official Use Only - General]
>>>>>
>>>>> For this case, we don’t have to take another global reformatting.
>>>>> These two formats can coexisting without the conflict.  We just allow
>>>>> the condense argus format in CSS. Also, update Uncrustify to not
>>>>> forcing each argument at its own line.
>>>>>
>>>>> The current Uncrustify behavior seems to me match the CCS spec. But
>>>>> this patch was sent to allow the multiple argus at the same line,
>>>>> which was not proposed to fix the issue in current Uncrustify. You
>>>>> sure we just close this issue?
>>>>>
>>>>> Abner
>>>>>
>>>>> *From:* devel@edk2.groups.io <mailto:devel@edk2.groups.io>
>>>>> <devel@edk2.groups.io <mailto:devel@edk2.groups.io>> *On Behalf Of
>>>>> *Michael D Kinney via groups.io
>>>>> *Sent:* Monday, November 14, 2022 1:36 AM
>>>>> *To:* devel@edk2.groups.io <mailto:devel@edk2.groups.io>; Chang, 
>>>>> Abner
>>>>> <Abner.Chang@amd.com <mailto:Abner.Chang@amd.com>>; Laszlo Ersek
>>>>> <lersek@redhat.com <mailto:lersek@redhat.com>>; Kubacki, Michael
>>>>> <michael.kubacki@microsoft.com
>>>>> <mailto:michael.kubacki@microsoft.com>>; Kinney, Michael D
>>>>> <michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>>
>>>>> *Subject:* Re: [edk2-devel] [edk2-CCodingStandardsSpecification PATCH
>>>>> 2/2] Source Files / Spacing / Multi-line func. calls: allow condensed
>>>>> arguments
>>>>>
>>>>>
>>>>>
>>>>> *Caution:*This message originated from an External Source. Use proper
>>>>> caution when opening attachments, clicking links, or responding.
>>>>>
>>>>> We do not want another global format change because that make git
>>>>> blame difficult to use.
>>>>>
>>>>> Are any clarifications required to describe the current Uncrustify
>>>>> behavior?  Or is the description correct?
>>>>>
>>>>> If the current description matches Uncristify behavior, then I
>>>>> recommend we close this issue as will not fix.
>>>>>
>>>>> Mike
>>>>>
>>>>> *From:* devel@edk2.groups.io <mailto:devel@edk2.groups.io>
>>>>> <devel@edk2.groups.io <mailto:devel@edk2.groups.io>> *On Behalf Of
>>>>> *Chang, Abner via groups.io
>>>>> *Sent:* Sunday, November 13, 2022 12:45 AM
>>>>> *To:* Kinney, Michael D <michael.d.kinney@intel.com
>>>>> <mailto:michael.d.kinney@intel.com>>; devel@edk2.groups.io
>>>>> <mailto:devel@edk2.groups.io>; Laszlo Ersek <lersek@redhat.com
>>>>> <mailto:lersek@redhat.com>>; Kubacki, Michael
>>>>> <michael.kubacki@microsoft.com 
>>>>> <mailto:michael.kubacki@microsoft.com>>
>>>>> *Subject:* Re: [edk2-devel] [edk2-CCodingStandardsSpecification PATCH
>>>>> 2/2] Source Files / Spacing / Multi-line func. calls: allow condensed
>>>>> arguments
>>>>>
>>>>> [AMD Official Use Only - General]
>>>>>
>>>>> Uncrustify can fix the first argument that is not at the indent with
>>>>> two space. It also can fix the first argument that is not at the new
>>>>> line.
>>>>>
>>>>> But it also makes each argument a new line if multiple args are
>>>>> condensed in one line. That is what we have to update Uncrustify 
>>>>> if we
>>>>> have this patch merged to CCS.
>>>>>
>>>>> +Michael Kubacki in loop.
>>>>>
>>>>> Abner
>>>>>
>>>>> *From:* Kinney, Michael D <michael.d.kinney@intel.com
>>>>> <mailto:michael.d.kinney@intel.com>>
>>>>> *Sent:* Sunday, November 13, 2022 9:58 AM
>>>>> *To:* devel@edk2.groups.io <mailto:devel@edk2.groups.io>; Chang, 
>>>>> Abner
>>>>> <Abner.Chang@amd.com <mailto:Abner.Chang@amd.com>>; Laszlo Ersek
>>>>> <lersek@redhat.com <mailto:lersek@redhat.com>>; Kinney, Michael D
>>>>> <michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>>
>>>>> *Subject:* RE: [edk2-devel] [edk2-CCodingStandardsSpecification PATCH
>>>>> 2/2] Source Files / Spacing / Multi-line func. calls: allow condensed
>>>>> arguments
>>>>>
>>>>>
>>>>>
>>>>> *Caution:*This message originated from an External Source. Use proper
>>>>> caution when opening attachments, clicking links, or responding.
>>>>>
>>>>> Is this exactly what Uncrustify does now?
>>>>>
>>>>> Mike
>>>>>
>>>>> *From:* devel@edk2.groups.io <mailto:devel@edk2.groups.io>
>>>>> <devel@edk2.groups.io <mailto:devel@edk2.groups.io>> *On Behalf Of
>>>>> *Chang, Abner via groups.io
>>>>> *Sent:* Saturday, November 12, 2022 5:36 PM
>>>>> *To:* Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>>;
>>>>> devel@edk2.groups.io <mailto:devel@edk2.groups.io>
>>>>> *Subject:* Re: [edk2-devel] [edk2-CCodingStandardsSpecification PATCH
>>>>> 2/2] Source Files / Spacing / Multi-line func. calls: allow condensed
>>>>> arguments
>>>>>
>>>>> Hi all,
>>>>> As we are going to release CCS 2.3, we would like to address some
>>>>> pending issues of CCS. For this, I think we can,
>>>>> - Still keep the one line per argument style in CCS although the
>>>>> multi-arguments in the one line style can cover this. This avoids
>>>>> confusion from readers and questions about if they can do the 
>>>>> one-line
>>>>> per argument style.
>>>>> - If the arguments are in different lines, the first argument must be
>>>>> indented with two spaces from the start of the function name or the
>>>>> member function name.
>>>>> How is this?
>>>>>
>>>>> Abner
>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>>
>>>
>>
>
>
> 
>
>