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

Tue Nov 15 02:38:59 UTC 2022

[AMD Official Use Only - General]

Seem we have the consensus to change this BZ to WONTFIX, reopen it if necessary after the further discussions. The "code > 80 columns" and "the multiple argus at one line" are not considered to be part of CCS v2.3 release also.

Abner

> -----Original Message-----
> From: Sean Brogan <spbrogan at outlook.com>
> Sent: Tuesday, November 15, 2022 3:09 AM
> To: devel at edk2.groups.io; mikuback at linux.microsoft.com; Kinney, Michael
> D <michael.d.kinney at intel.com>; Chang, Abner <Abner.Chang at amd.com>;
> Laszlo Ersek <lersek at redhat.com>; Kubacki, Michael
> <michael.kubacki at microsoft.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.
> 
> 
> 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk
> >>
> 2.groups.io%2Fg%2Fdevel%2Fmessage%2F96038&data=05%7C01%7Cab
> ner.ch
> >>
> ang%40amd.com%7C1f9c0bc3c6c747f1297308dac673afe9%7C3dd8961fe4884
> e608e
> >>
> 11a82d994e183d%7C0%7C0%7C638040497500028713%7CUnknown%7CTWFp
> bGZsb3d8e
> >>
> yJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D
> %7C3
> >>
> 000%7C%7C%7C&sdata=TEqQG9CS8IgF4x33PsIufmwLfM6Tpj%2Be8JQx
> 9ZxeqHo%
> >> 3D&reserved=0
> >>
> >>
> >> Mike
> >>
> >>> -----Original Message-----
> >>> From: devel at edk2.groups.io <devel at edk2.groups.io> On Behalf Of
> >>> Michael Kubacki
> >>> Sent: Monday, November 14, 2022 10:05 AM
> >>> To: devel at edk2.groups.io; Kinney, Michael D
> >>> <michael.d.kinney at intel.com>; abner.chang at amd.com; Laszlo Ersek
> >>> <lersek at redhat.com>; Kubacki, Michael
> >>> <michael.kubacki at 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgith
> ub.com%2Ftianocore%2Fedk2%2Fblob%2Fmaster%2F.pytool%2FPlugin%2FU
> ncrustifyCheck%2Funcrustify.cfg%23L19&data=05%7C01%7Cabner.chan
> g%40amd.com%7C1f9c0bc3c6c747f1297308dac673afe9%7C3dd8961fe4884e6
> 08e11a82d994e183d%7C0%7C0%7C638040497500028713%7CUnknown%7CT
> WFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLC
> JXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=6CpnXs63WTGicnIznfXtG
> MZHWjgq24Sz3yC8uWzEwQA%3D&reserved=0.
> >>>>
> >>>>
> >>>>
> >>>> 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fg
> >>>>
> ithub.com%2Fmakubacki%2Fedk2%2Ftree%2Funcrustify_80_column_chang
> e&a
> >>>>
> mp;data=05%7C01%7Cabner.chang%40amd.com%7C1f9c0bc3c6c747f1297308
> dac
> >>>>
> 673afe9%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C6380404975
> 0002
> >>>>
> 8713%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2lu
> MzIiLC
> >>>>
> JBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=7OJjrlwy
> Eq%2
> >>>> FPdbYEZFFsVvHsYs0ir0cVuLMWy4wxLEM%3D&reserved=0
> >>>>
> >>>> 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fg
> >>>>
> ithub.com%2Fmakubacki%2Fedk2%2Fblob%2Funcrustify_80_column_chang
> e%2
> >>>> FMdePkg%2FInclude%2FIndustryStandard%2FAcpi30.h%23L702-
> %23L704&
> >>>>
> data=05%7C01%7Cabner.chang%40amd.com%7C1f9c0bc3c6c747f1297308dac
> 673
> >>>>
> afe9%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C6380404975000
> 2871
> >>>>
> 3%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzI
> iLCJBT
> >>>>
> iI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=1nCH5GoBl
> LU9Lb0
> >>>> r%2FVzuEaPg2mXTS3JGpLi9UBwRTYk%3D&reserved=0
> >>>>
> >>>>
> >>>> 2.
> >>>>
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fg
> >>>>
> ithub.com%2Fmakubacki%2Fedk2%2Fblob%2Funcrustify_80_column_chang
> e%2
> >>>>
> FMdePkg%2FInclude%2FIndustryStandard%2FIpmiNetFnApp.h%23L1023-
> %23L1
> >>>>
> 031&data=05%7C01%7Cabner.chang%40amd.com%7C1f9c0bc3c6c747f1
> 2973
> >>>>
> 08dac673afe9%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C63804
> 0497
> >>>>
> 500028713%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIj
> oiV2luM
> >>>>
> zIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=Sw
> %2BVP
> >>>> dtwl8ydIWSq3B80hDz2luABjdXsP91O2WcJ0aA%3D&reserved=0
> >>>>
> >>>>
> >>>>
> >>>> 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fg
> >>>>
> ithub.com%2Fmakubacki%2Fedk2%2Ftree%2Ffunc_arg_formatting_demo&
> amp;
> >>>>
> data=05%7C01%7Cabner.chang%40amd.com%7C1f9c0bc3c6c747f1297308dac
> 673
> >>>>
> afe9%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C6380404975000
> 2871
> >>>>
> 3%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzI
> iLCJBT
> >>>>
> iI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=Y2MTi5v5Fl
> PuEdx
> >>>> 5Lmg8%2Fm15TK7rNwlYapK%2FBExHC6o%3D&reserved=0
> >>>>
> >>>> 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 at edk2.groups.io <devel at edk2.groups.io> *On Behalf
> Of
> >>>>> *Chang, Abner via groups.io
> >>>>> *Sent:* Sunday, November 13, 2022 5:10 PM
> >>>>> *To:* devel at edk2.groups.io; Kinney, Michael D
> >>>>> <michael.d.kinney at intel.com>; Laszlo Ersek <lersek at redhat.com>;
> >>>>> Kubacki, Michael <michael.kubacki at 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 at edk2.groups.io <mailto:devel at edk2.groups.io>
> >>>>> <devel at edk2.groups.io <mailto:devel at edk2.groups.io>> *On Behalf
> Of
> >>>>> *Michael D Kinney via groups.io
> >>>>> *Sent:* Monday, November 14, 2022 1:36 AM
> >>>>> *To:* devel at edk2.groups.io <mailto:devel at edk2.groups.io>; Chang,
> >>>>> Abner <Abner.Chang at amd.com <mailto:Abner.Chang at amd.com>>;
> Laszlo
> >>>>> Ersek <lersek at redhat.com <mailto:lersek at redhat.com>>; Kubacki,
> >>>>> Michael <michael.kubacki at microsoft.com
> >>>>> <mailto:michael.kubacki at microsoft.com>>; Kinney, Michael D
> >>>>> <michael.d.kinney at intel.com <mailto:michael.d.kinney at 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 at edk2.groups.io <mailto:devel at edk2.groups.io>
> >>>>> <devel at edk2.groups.io <mailto:devel at 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 at intel.com
> >>>>> <mailto:michael.d.kinney at intel.com>>; devel at edk2.groups.io
> >>>>> <mailto:devel at edk2.groups.io>; Laszlo Ersek <lersek at redhat.com
> >>>>> <mailto:lersek at redhat.com>>; Kubacki, Michael
> >>>>> <michael.kubacki at microsoft.com
> >>>>> <mailto:michael.kubacki at 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 at intel.com
> >>>>> <mailto:michael.d.kinney at intel.com>>
> >>>>> *Sent:* Sunday, November 13, 2022 9:58 AM
> >>>>> *To:* devel at edk2.groups.io <mailto:devel at edk2.groups.io>; Chang,
> >>>>> Abner <Abner.Chang at amd.com <mailto:Abner.Chang at amd.com>>;
> Laszlo
> >>>>> Ersek <lersek at redhat.com <mailto:lersek at redhat.com>>; Kinney,
> >>>>> Michael D <michael.d.kinney at intel.com
> >>>>> <mailto:michael.d.kinney at 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 at edk2.groups.io <mailto:devel at edk2.groups.io>
> >>>>> <devel at edk2.groups.io <mailto:devel at edk2.groups.io>> *On Behalf
> Of
> >>>>> *Chang, Abner via groups.io
> >>>>> *Sent:* Saturday, November 12, 2022 5:36 PM
> >>>>> *To:* Laszlo Ersek <lersek at redhat.com
> <mailto:lersek at redhat.com>>;
> >>>>> devel at edk2.groups.io <mailto:devel at 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
> >>>>>
> >>>>>
> >>>>
> >>>>
> >>>>
> >>>>
> >>>
> >>>
> >>>
> >>>
> >>
> >
> >
> > 
> >
> >

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#96373): https://edk2.groups.io/g/devel/message/96373
Mute This Topic: https://groups.io/mt/30907266/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-