From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from rn-mailsvcp-ppex-lapp14.apple.com (rn-mailsvcp-ppex-lapp14.apple.com [17.179.253.33]) by mx.groups.io with SMTP id smtpd.web11.505.1633640976935590291 for ; Thu, 07 Oct 2021 14:09:37 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@apple.com header.s=20180706 header.b=Dwf6meqp; spf=pass (domain: apple.com, ip: 17.179.253.33, mailfrom: afish@apple.com) Received: from pps.filterd (rn-mailsvcp-ppex-lapp14.rno.apple.com [127.0.0.1]) by rn-mailsvcp-ppex-lapp14.rno.apple.com (8.16.1.2/8.16.1.2) with SMTP id 197L8dEK027484; Thu, 7 Oct 2021 14:09:34 -0700 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=apple.com; h=from : message-id : content-type : mime-version : subject : date : in-reply-to : cc : to : references; s=20180706; bh=LF1jekWqES8e+pcCpVCJtO/+TrD+4qhZBBbOK075QEU=; b=Dwf6meqp2zXFfIipC7wEnDInUS3MISwQqkF9P3h6VnFm7iW9irlt+qh03zcM2NEy3OJx SpbGXLXsQivXJbKEO996L4EclO2+dBD62R9RP3Wa1XprT5+5OweuvokQdyB0q8Bdmlcq 82LiPa0pL05eWviG4I8hl1RJcRNAwyBqvCLiQIY0vrxwYvWsFfm5DVUV9R4hFL0XOiGw 5u2Yd4WtzPtNcp4CvInu0sUYskR58+OGKXknq6LrVcV2Avze9BGcj0IKip02jpMtAYSw x89lq+PMteJ+984DyVfdAr+HIsmY1XW/v6ExRQHaaTQ63nW/Aus7+QWFR8AYbV11nuMC rA== Received: from ma-mailsvcp-mta-lapp01.corp.apple.com (ma-mailsvcp-mta-lapp01.corp.apple.com [10.226.18.133]) by rn-mailsvcp-ppex-lapp14.rno.apple.com with ESMTP id 3bj3wfm83d-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NO); Thu, 07 Oct 2021 14:09:34 -0700 Received: from ma-mailsvcp-mmp-lapp02.apple.com (ma-mailsvcp-mmp-lapp02.apple.com [17.32.222.15]) by ma-mailsvcp-mta-lapp01.corp.apple.com (Oracle Communications Messaging Server 8.1.0.12.20210903 64bit (built Sep 3 2021)) with ESMTPS id <0R0M00NBMLFYNY40@ma-mailsvcp-mta-lapp01.corp.apple.com>; Thu, 07 Oct 2021 14:09:34 -0700 (PDT) Received: from process_milters-daemon.ma-mailsvcp-mmp-lapp02.apple.com by ma-mailsvcp-mmp-lapp02.apple.com (Oracle Communications Messaging Server 8.1.0.12.20210903 64bit (built Sep 3 2021)) id <0R0M00Q00LA2LK00@ma-mailsvcp-mmp-lapp02.apple.com>; Thu, 07 Oct 2021 14:09:34 -0700 (PDT) X-Va-A: X-Va-T-CD: cb83049425a79c8a5fb9f1dafa0fda92 X-Va-E-CD: 574fda742400e238a02c5048d028c5dd X-Va-R-CD: b4e96f8f3d52136df210f2e6a101920e X-Va-CD: 0 X-Va-ID: c42d8caf-6e81-44ad-8178-f2f37cd3c2ac X-V-A: X-V-T-CD: cb83049425a79c8a5fb9f1dafa0fda92 X-V-E-CD: 574fda742400e238a02c5048d028c5dd X-V-R-CD: b4e96f8f3d52136df210f2e6a101920e X-V-CD: 0 X-V-ID: 819fad31-97c3-474a-8fd4-cb9296aac134 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.391,18.0.790 definitions=2021-10-07_04:2021-10-07,2021-10-07 signatures=0 Received: from [17.234.148.236] (unknown [17.234.148.236]) by ma-mailsvcp-mmp-lapp02.apple.com (Oracle Communications Messaging Server 8.1.0.12.20210903 64bit (built Sep 3 2021)) with ESMTPSA id <0R0M00W06LFPXX00@ma-mailsvcp-mmp-lapp02.apple.com>; Thu, 07 Oct 2021 14:09:33 -0700 (PDT) From: "Andrew Fish" Message-id: MIME-version: 1.0 (Mac OS X Mail 14.0 \(3654.20.0.2.1\)) Subject: Re: [edk2-devel] Progress on getting Uncrustify working for EDK2? Date: Thu, 07 Oct 2021 17:09:22 -0400 In-reply-to: Cc: edk2-devel-groups-io , Mike Kinney , Leif Lindholm , "mikuback@linux.microsoft.com" , "rebecca@nuviainc.com" , Michael Kubacki , Bret Barkelew To: =?utf-8?Q?Marvin_H=C3=A4user?= References: <07a6ecff-f7bf-083a-f24d-246ca6c7988b@nuviainc.com> <2679bfa3-b4ec-d8e9-7e56-54ebe42d9001@posteo.de> <9fe0f984-db9d-9aec-0b44-5d30791a2855@linux.microsoft.com> <20211007104813.wa4rmfsqgcpvnzwt@leviathan> <07d5c8bc-40b2-4e99-3b3d-4c8ac4e14220@posteo.de> <438B4D66-2CFB-45E3-AF75-42342F0B1E67@apple.com> X-Mailer: Apple Mail (2.3654.20.0.2.1) X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.391,18.0.790 definitions=2021-10-07_04:2021-10-07,2021-10-07 signatures=0 Content-type: multipart/alternative; boundary="Apple-Mail=_4C8FF348-A10D-4139-BC54-7BFDF459BA11" --Apple-Mail=_4C8FF348-A10D-4139-BC54-7BFDF459BA11 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 > On Oct 7, 2021, at 1:43 PM, Marvin H=C3=A4user wrote= : >=20 > Hey Mike, > Hey Andrew, >=20 > I'll just reply to both mails at once :) >=20 > On 07/10/2021 19:36, Andrew Fish wrote: >>=20 >>=20 >>> On Oct 7, 2021, at 1:19 PM, Michael D Kinney wrote: >>>=20 >>> Hi Marvin, >>>=20 >>> Some comments below. >>>=20 >>> Mike >>>=20 >>>> -----Original Message----- >>>> From:devel@edk2.groups.io On Behalf Of Marvin H= =C3=A4user >>>> Sent: Thursday, October 7, 2021 11:31 AM >>>> To: Leif Lindholm ;devel@edk2.groups.io;mikuback@li= nux.microsoft.com >>>> Cc:rebecca@nuviainc.com; Michael Kubacki ; Bret Barkelew ; >>>> Kinney, Michael D >>>> Subject: Re: [edk2-devel] Progress on getting Uncrustify working for E= DK2? >>>>=20 >>>> Good day, >>>>=20 >>>> +1, but while you're at it, can we have arguments not align to the >>>> function name... >>>>=20 >>>> Status =3D Test ( >>>> a >>>> ); >>>>=20 >>>> ... but to the next natural indentation level? >>>>=20 >>>> Status =3D Test ( >>>> a >>>> ); >>>>=20 >>>> Basically no IDE I have seen supports EDK II's style, and I wouldn't b= e >>>> keen on writing known-broken style to then rely on Uncrustify to fix i= t. >>>>=20 >>>> I also have heard some controversy regarding casts off-list, where som= e >>>> prefer no spaces after casts to stress the evaluation order, and some >>>> prefer spaces to have clearer visuals (as a cast *ideally* would be >>>> something rare that requires good justification). Just throwing that o= ut >>>> there. >>>>=20 >>>>=20 >>>> For things unrelated to autoformat (so semi-offtopic) but still releva= nt >>>> to the coding spec: >>>>=20 >>>> 1. Allow STATIC functions (if the debugging concerns are still relevan= t, >>>> there could be another level of indirection, like RELEASE_STATIC)? >>>=20 >>> Debugging concerns are no longer relevant. The suggestion in the BZ be= low >>> is to remove the STATIC macro and allow EDK II sources to add 'static' >>> to any functions it makes sense to use on. >>>=20 >>> https://bugzilla.tianocore.org/show_bug.cgi?id=3D1766 >=20 > Thanks! I'd keep STATIC actually just for the sake of not doing no-op cha= nges that do not really do anything and for consistency with CONST, but wha= tever works really. >=20 >>>=20 >>>>=20 >>>> 2. Allow variable assignments on definition (basically non-static CONS= T >>>> variables are banned...)? >>>=20 >>> Are referring to use of pre-initialized CONST variables declared within >>> a function? I think Bret brought this topic up when implementing some >>> unit tests and the suggestion to pass ECCC was to promote them to >>> pre-initialized CONST global variables. >=20 > Yes. >=20 >>>=20 >>> The challenges we have seen in the past with pre-initialized variables = within >>> a function is that they can cause compilers to inject use of memcpy() c= alls, >>> especially if the variable being initialized on the stack is a structur= e. >>> These cause build breaks today. >>=20 >> This issue is independent of CONST. I=E2=80=99m not sure a coding style = tool is smart enough to catch this generically? You need an understanding o= f C types to know if the local variable assignment is going to trigger a me= mcpy(). >>=20 >> What I=E2=80=99ve seen in the real world is the firmware compiles with -= Os or LTO to fit int he ROM for DEBUG and RELEASE, and the optimizer optimi= zes away the call to memcpy. Then if you try to build NOOPT (or over ride t= he compiler flags on an individual driver/lib) you fail to link as only the= NOOPT build injects the memcpy. >=20 > +1 >=20 >>=20 >> Thus I think the best way to enforce this rule is to compile a project N= OOPT. I=E2=80=99m trying to remember are there flags to built to tell it to= compile and skip the FD construction? Maybe we should advocate platforms a= dd a NOOPT build target that just compiles the code, but does not create th= e FD? >=20 > I know there were stability concerns with intrinsics in the past, but mem= cpy() is in the standard, and the rest remained stable to my knowledge. May= be it's time to fix the issues at the root? Works for us: > https://github.com/acidanthera/OpenCorePkg/tree/master/Library/OcCompiler= IntrinsicsLib >=20 Marvin, Good point. This would make the rule moot. So maybe just removing the requi= rement would be the easiest long term fix.=20 Other embedded projects I know of do this too, and as you point out the com= pilers keep these APIs standard for folks the provide their own runtimes. Thanks, Andrew Fish > Best regards, > Marvin >=20 >>=20 >> Thanks, >>=20 >> Andrew Fish >>=20 >>>=20 >>>>=20 >>>> 3. Allow variable declarations at any scope (I had some nasty shadowin= g >>>> bugs before, probably prohibit shadowing with warnings)? >>>=20 >>> By shadowing do you mean the declaration of the same variable name in >>> multiple scoped within the same function? >>>=20 >>>>=20 >>>> 4. Require that exactly all function declarations and all function >>>> definitions with no prior declaration must be documented (first >>>> direction is enforcing docs, second is prohibiting doc duplication, I'= ve >>>> seen them go out-of-sync plenty of times)? >>>=20 >>> I agree that this can reduce duplication and sync issues. The uncrusti= fy >>> tool being discussed here could not help clean this up or enforce this >>> type of rule. It is a good topic, but may need to be split out into it= s >>> own thread. >>>=20 >>>>=20 >>>> The latter bunch would not require any autoformat rules or reformatati= on >>>> of existing code, but would be target only new submissions in my >>>> opinion. Thoughts? >>>>=20 >>>>=20 >>>> Thanks for your efforts! >>>>=20 >>>> Best regards, >>>> Marvin >>>>=20 >>>>=20 >>>> Am 07.10.2021 um 12:48 schrieb Leif Lindholm: >>>>> Hi Michael, >>>>>=20 >>>>> Apologies, I've owed you a response (promised off-list) for a while >>>>> now. >>>>>=20 >>>>> First, let me say I hugely appreciate this effort. Apart from alignin= g >>>>> the codebase(s), this will reduce manual reviewing effort >>>>> substantially, as well as cutting down on number of rework cycles for >>>>> developers. >>>>>=20 >>>>> Looking at the changes to (well, the comments in) uncrustify, this >>>>> seems to be constrained to: >>>>> - Newline after '(' for multi-line function calls. >>>>> - Dealing with "(("/"))" for DEBUG macros. >>>>> - Function pointer typedefs: >>>>> - typedef\nEFIAPI >>>>> - closing parentheses indentation >>>>>=20 >>>>> I don't think I've made any secret over the years that I am not a >>>>> massive fan of the EDK2 coding style in general. So I think for any >>>>> of its quirks that are substantial enough that they require not just >>>>> custom configuration but actual new function added to existing code >>>>> conformance tools, this would be an excellent point to sanitise the >>>>> coding style instead. >>>>>=20 >>>>> Taking these in order: >>>>>=20 >>>>> Newline after '(' >>>>> ----------------- >>>>> I think we already reached a level of flexibility around this, where >>>>> we don't actually enforce this (or single argument per >>>>> line). Personally, I'd be happy to update the coding style as >>>>> required instead. >>>>>=20 >>>>> DEBUG macro parentheses >>>>> ----------------------- >>>>> How does uncrustify treat DEBUG macros without this modification? >>>>> Do we start getting everything turned into multi-level indented >>>>> multi-line statements without this change? >>>>>=20 >>>>> Function pointer typedefs: >>>>> -------------------------- >>>>> I don't see that function pointer typedefs need to rigidly follow the >>>>> same pattern as the declaration of functions implementing them. Could >>>>> we update the coding style (if needed) instead? >>>>>=20 >>>>> Best Regards, >>>>>=20 >>>>> Leif >>>>>=20 >>>>> On Mon, Aug 16, 2021 at 16:00:38 -0400, Michael Kubacki wrote: >>>>>> The edk2 branch was created here: >>>>>> https://github.com/makubacki/edk2/tree/uncrustify_poc_2 >>>>>>=20 >>>>>> We have a Project Mu fork with custom changes to the Uncrustify tool= to help >>>>>> comply with EDK II formatting here: >>>>>> https://dev.azure.com/projectmu/_git/Uncrustify >>>>>>=20 >>>>>> The latest information about the status and how to experiment with t= he >>>>>> configuration file and the tool are in that fork: >>>> https://dev.azure.com/projectmu/Uncrustify/_wiki/wikis/Uncrustify.wiki= /1/Project-Mu-(EDK-II)-Fork-Readme >>>>>>=20 >>>>>> That said, I have also finished a CI plugin to run Uncrustify that s= hould be >>>>>> ready soon to initially deploy in Project Mu. Before doing so, I am = trying >>>>>> to settle on an initial configuration file that less strictly but mo= re >>>>>> reliably formats the code than in the examples in those branches. Fo= r >>>>>> example, remove heuristics that when run against the same set of cod= e >>>>>> multiple times can produce different results. An example would be a = rule >>>>>> that reformats code because it exceeds a specified column width on o= ne run >>>>>> but on the next run that reformatted code triggers a different rule = to >>>>>> further align the code and so on. At least initially, some rules mig= ht be >>>>>> tweaked in a more conservative approach that can be tightened in the= future. >>>>>> Once this configuration file is ready, we will baseline Project Mu c= ode as >>>>>> an example and turn on the plugin. The CI plugin runs Uncrustify aga= inst >>>>>> modified files and if there's any changes, indicating a formatting >>>>>> deviation, the diff chunks are saved in a log so they can be viewed = as a >>>>>> build artifact. >>>>>>=20 >>>>>> I am making progress on the updated config file and I should be able= to post >>>>>> a "uncrustify_poc_3" branch soon with the results. >>>>>>=20 >>>>>> Regarding indentation, Marvin is right that Uncrustify cannot suppor= t edk2 >>>>>> indentation style out-of-box. Some changes are made in that fork to = handle >>>>>> the formatting. At this point, it can handle the indentation in the = cases >>>>>> I've seen. Uncrustify does potentially give us the ability to massiv= ely >>>>>> deploy changes across the codebase in case a decision were made to c= hange >>>>>> the style. >>>>>>=20 >>>>>> Thanks, >>>>>> Michael >>>>>>=20 >>>>>> On 8/16/2021 3:39 PM, Marvin H=C3=A4user wrote: >>>>>>> Hey Rebecca, >>>>>>>=20 >>>>>>> I think even Uncrustify has issues with the EDK II indentation styl= e. >>>>>>> You might want to check the UEFI Talkbox Discord server, I had a br= ief >>>>>>> chat with Michael about it there. I don't think realistically any t= ool >>>>>>> supports EDK II's indentation style however, so I'd propose it is >>>>>>> changed. This could be for new submissions only, or actually the en= tire >>>>>>> codebase could be reformatted at once with a good tool setup. While= this >>>>>>> screws with git blame, the (to my understanding) decided on CRLF ->= LF >>>>>>> change does that anyway, so at least two evils could be dealt with = in >>>>>>> one go really. >>>>>>>=20 >>>>>>> Best regards, >>>>>>> Marvin >>>>>>>=20 >>>>>>> On 16/08/2021 21:34, Rebecca Cran wrote: >>>>>>>>=20 >>>>>>>> cc devel@ . >>>>>>>>=20 >>>>>>>> On 8/16/21 1:33 PM, Rebecca Cran wrote: >>>>>>>>>=20 >>>>>>>>> I noticed a message on Twitter about an idea of using Uncrustify >>>>>>>>> for EDK2 instead of the ECC tool, and came across https://www.mai= l- >>>> archive.com/search?l=3Ddevel@edk2.groups.io&q=3Dsubject:%22Re%5C%3A+%5= C%5Bedk2%5C- >>>> devel%5C%5D+TianoCore+Community+Meeting+Minutes+%5C-+2%5C%2F4%22&o=3Dn= ewest&f=3D1 >>>>>>>>> . >>>>>>>>>=20 >>>>>>>>> I was wondering if there's been any progress on it that I could >>>>>>>>> check out? >>>>>>>>>=20 >>>>>>>>>=20 >>>>>>>>> Michael Kubacki: in that message, you said: >>>>>>>>>=20 >>>>>>>>> "I'm planning to put up a branch that we can use as a reference >>>>>>>>> for a conversation around uncrustify in the next couple of >>>>>>>>> weeks." >>>>>>>>>=20 >>>>>>>>>=20 >>>>>>>>> Did you end up creating that branch, and if so could you provide >>>>>>>>> a link to it please? >>>>>>>>>=20 >>>>>>>>>=20 >>>>>>>>> -- >>>>>>>>> Rebecca Cran >>>>>>>>>=20 >>>>>>>>=20 >>>>>>>=20 >>>>>>>=20 >>>>>>>=20 >>>>>>>=20 >>>>>>>=20 >>>>>>=20 >>>>>>=20 >>>>>>=20 >>>>>>=20 >>>>>>=20 >>>>>>=20 >>>>=20 >>>>=20 >>>>=20 >>>>=20 >>>=20 >>>=20 >>>=20 >>>=20 --Apple-Mail=_4C8FF348-A10D-4139-BC54-7BFDF459BA11 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8

On Oct 7, 202= 1, at 1:43 PM, Marvin H=C3=A4user <mhaeuser@posteo.de> wrote:

H= ey Mike,
Hey Andrew,

I'll just reply to both mails at once :)

On 0= 7/10/2021 19:36, Andrew Fish wrote:


On Oct 7, 2021, at 1:19 PM, Michael D Kinney <michael.d.kinney@intel.co= m> wrote:

Hi Marvin,

Some comments below.

Mike

-----Original Mes= sage-----
From:devel@edk2.groups.io<devel@edk2.groups.io> On Behalf Of Marvin H=C3=A4user
Sent: Thursday, October 7, 2021 11:31 AM
To: Leif L= indholm <leif@nuviainc.c= om>;devel@edk2.gr= oups.io;miku= back@linux.microsoft.com
Cc:rebecca@nuviainc.com; Michael Kubacki <Michael.Kubacki@micros= oft.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>;
Kinney= , Michael D <mi= chael.d.kinney@intel.com>
Subject: Re: [edk2-devel] Pr= ogress on getting Uncrustify working for EDK2?

Good day,

+1, but while you're at it, can we = have arguments not align to the
function name...

  Status =3D Test (
  &n= bsp;          a
           &nb= sp; );

... but to the next natural indent= ation level?

  Status =3D Test (
    a
    = );

Basically no IDE I have seen supports EDK I= I's style, and I wouldn't be
keen on writing known-broken sty= le to then rely on Uncrustify to fix it.

I als= o have heard some controversy regarding casts off-list, where some
prefer no spaces after casts to stress the evaluation order, and some=
prefer spaces to have clearer visuals (as a cast *ideally* w= ould be
something rare that requires good justification). Jus= t throwing that out
there.


For things unrelated to autoformat (so semi-offtopic) but still r= elevant
to the coding spec:

1. A= llow STATIC functions (if the debugging concerns are still relevant,
there could be another level of indirection, like RELEASE_STATIC)?<= br class=3D"">

Debugging concerns are no longer = relevant.  The suggestion in the BZ below
is to remove t= he STATIC macro and allow EDK II sources to add 'static'
to a= ny functions it makes sense to use on.

https= ://bugzilla.tianocore.org/show_bug.cgi?id=3D1766

Thanks! I'd= keep STATIC actually just for the sake of not doing no-op changes that do = not really do anything and for consistency with CONST, but whatever works r= eally.


2. Allow v= ariable assignments on definition (basically non-static CONST
variables are banned...)?

Are re= ferring to use of pre-initialized CONST variables declared within
a function?  I think Bret brought this topic up when implementin= g some
unit tests and the suggestion to pass ECCC was to prom= ote them to
pre-initialized CONST global variables.

Yes.

<= br class=3D"">The challenges we have seen in the past with pre-initialized = variables within
a function is that they can cause compilers = to inject use of memcpy() calls,
especially if the variable b= eing initialized on the stack is a structure.
These cause bui= ld breaks today.

This issue is in= dependent of CONST. I=E2=80=99m not sure a coding style tool is smart enoug= h to catch this generically? You need an understanding of C types to know i= f the local variable assignment is going to trigger a memcpy().

What I=E2=80=99ve seen in the real world is the firmware = compiles with -Os or LTO to fit int he ROM for DEBUG and RELEASE, and the o= ptimizer optimizes away the call to memcpy. Then if you try to build NOOPT = (or over ride the compiler flags on an individual driver/lib) you fail to l= ink as only the NOOPT build injects the memcpy.
=
+1


Thus I think the best way to enforce this rule is to c= ompile a project NOOPT. I=E2=80=99m trying to remember are there flags to b= uilt to tell it to compile and skip the FD construction? Maybe we should ad= vocate platforms add a NOOPT build target that just compiles the code, but = does not create the FD?

I know there were stability concerns with intrinsics = in the past, but memcpy() is in the standard, and the rest remained stable = to my knowledge. Maybe it's time to fix the issues at the root? Works for u= s:
https://github.com/acidanthera/O= penCorePkg/tree/master/Library/OcCompilerIntrinsicsLib


Marvin,

Good point. This wo= uld make the rule moot. So maybe just removing the requirement would be the= easiest long term fix. 

Other emb= edded projects I know of do this too, and as you point out the compilers ke= ep these APIs standard for folks the provide their own runtimes.
=
Thanks,

Andre= w Fish

Best regards,
Marvin


Thanks,

Andrew Fish



3. Allow variable declarations at any scope (I had some nas= ty shadowing
bugs before, probably prohibit shadowing with wa= rnings)?

By shadowing do you mean= the declaration of the same variable name in
multiple scoped= within the same function?


4. Require that exactly all function decla= rations and all function
definitions with no prior declaratio= n must be documented (first
direction is enforcing docs, seco= nd is prohibiting doc duplication, I've
seen them go out-of-s= ync plenty of times)?

I agree tha= t this can reduce duplication and sync issues.  The uncrustify
tool being discussed here could not help clean this up or enforce th= is
type of rule.  It is a good topic, but may need to be= split out into its
own thread.

=

The latter bunch would = not require any autoformat rules or reformatation
of existing= code, but would be target only new submissions in my
opinion= . Thoughts?


Thanks for your eff= orts!

Best regards,
Marvin


Am 07.10.2021 um 12:48 schrieb Leif = Lindholm:
Hi Michael,
Apologies, I've owed you a response (promised of= f-list) for a while
now.

First, = let me say I hugely appreciate this effort. Apart from aligning
the codebase(s), this will reduce manual reviewing effort
= substantially, as well as cutting down on number of rework cycles for
developers.

Looking at the changes to= (well, the comments in) uncrustify, this
seems to be constra= ined to:
- Newline after '(' for multi-line function calls.- Dealing with "(("/"))" for DEBUG macros.
- Fun= ction pointer typedefs:
  - typedef\nEFIAPI
  - closing parentheses indentation

I don't think I've made any secret over the years that I am not amassive fan of the EDK2 coding style in general. So I think for= any
of its quirks that are substantial enough that they requ= ire not just
custom configuration but actual new function add= ed to existing code
conformance tools, this would be an excel= lent point to sanitise the
coding style instead.

Taking these in order:

Newlin= e after '('
-----------------
I think we alread= y reached a level of flexibility around this, where
we don't = actually enforce this (or single argument per
line). Personal= ly, I'd be happy to update the coding style as
required inste= ad.

DEBUG macro parentheses
----= -------------------
How does uncrustify treat DEBUG macros wi= thout this modification?
Do we start getting everything turne= d into multi-level indented
multi-line statements without thi= s change?

Function pointer typedefs:
--------------------------
I don't see that function po= inter typedefs need to rigidly follow the
same pattern as the= declaration of functions implementing them. Could
we update = the coding style (if needed) instead?

Best Reg= ards,

Leif

On Mon= , Aug 16, 2021 at 16:00:38 -0400, Michael Kubacki wrote:
The edk2 branch was created here:
https://github.com/makubacki/edk2/tree/uncrustify_poc_2

We have a Project Mu fork with custom changes to th= e Uncrustify tool to help
comply with EDK II formatting here:=
https://dev.azure.com/projectmu/_git/Uncrustify

The latest information about the status and how to experim= ent with the
configuration file and the tool are in that fork= :
https://dev.azure.com/projectmu/Uncrustify/_wiki/wikis/= Uncrustify.wiki/1/Project-Mu-(EDK-II)-Fork-Readme

That said, I have also finished a CI plugin to run Uncrustify that s= hould be
ready soon to initially deploy in Project Mu. Before= doing so, I am trying
to settle on an initial configuration = file that less strictly but more
reliably formats the code th= an in the examples in those branches. For
example, remove heu= ristics that when run against the same set of code
multiple t= imes can produce different results. An example would be a rule
that reformats code because it exceeds a specified column width on one ru= n
but on the next run that reformatted code triggers a differ= ent rule to
further align the code and so on. At least initia= lly, some rules might be
tweaked in a more conservative appro= ach that can be tightened in the future.
Once this configurat= ion file is ready, we will baseline Project Mu code as
an exa= mple and turn on the plugin. The CI plugin runs Uncrustify against
modified files and if there's any changes, indicating a formattingdeviation, the diff chunks are saved in a log so they can be vi= ewed as a
build artifact.

I am m= aking progress on the updated config file and I should be able to post
a "uncrustify_poc_3" branch soon with the results.
=
Regarding indentation, Marvin is right that Uncrustify canno= t support edk2
indentation style out-of-box. Some changes are= made in that fork to handle
the formatting. At this point, i= t can handle the indentation in the cases
I've seen. Uncrusti= fy does potentially give us the ability to massively
deploy c= hanges across the codebase in case a decision were made to change
the style.

Thanks,
Michael=

On 8/16/2021 3:39 PM, Marvin H=C3=A4user wrot= e:
Hey Rebecca,

I think even Uncrustify has issues with the EDK II ind= entation style.
You might want to check the UEFI Talkbox Disc= ord server, I had a brief
chat with Michael about it there. I= don't think realistically any tool
supports EDK II's indenta= tion style however, so I'd propose it is
changed. This could = be for new submissions only, or actually the entire
codebase = could be reformatted at once with a good tool setup. While this
screws with git blame, the (to my understanding) decided on CRLF -> L= F
change does that anyway, so at least two evils could be dea= lt with in
one go really.

Best r= egards,
Marvin

On 16/08/2021 21:= 34, Rebecca Cran wrote:
=
cc devel@ .

On 8/16/21 1:33 PM,= Rebecca Cran wrote:
I noticed a message on Twitter about an idea of using Uncrustif= y
for EDK2 instead of the ECC tool, and came across https://w= ww.mail-
=
archive.com/search?l=3Ddevel@edk2.groups.io&q=3Dsubject:%2= 2Re%5C%3A+%5C%5Bedk2%5C-
devel%5C%5D+TianoCore+Community+Meet= ing+Minutes+%5C-+2%5C%2F4%22&o=3Dnewest&f=3D1
.

I was wondering = if there's been any progress on it that I could
check out?

Michael Kubacki: in that message,= you said:

"I'm planning to put up a branch th= at we can use as a reference
for a conversation around uncrus= tify in the next couple of
weeks."


Did you end up creating that branch, and if so could you = provide
a link to it please?

--
Rebecca Cran





















--Apple-Mail=_4C8FF348-A10D-4139-BC54-7BFDF459BA11--