From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mx.groups.io with SMTP id smtpd.web10.755.1637106891790209185 for ; Tue, 16 Nov 2021 15:54:51 -0800 Authentication-Results: mx.groups.io; dkim=fail reason="body hash did not verify" header.i=@linux.microsoft.com header.s=default header.b=TJCO7fgm; spf=pass (domain: linux.microsoft.com, ip: 13.77.154.182, mailfrom: mikuback@linux.microsoft.com) Received: from [10.0.0.19] (c-73-27-179-174.hsd1.fl.comcast.net [73.27.179.174]) by linux.microsoft.com (Postfix) with ESMTPSA id AD0AB20C5A74; Tue, 16 Nov 2021 15:54:50 -0800 (PST) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com AD0AB20C5A74 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1637106890; bh=1/97O+2kBJP+8I6aNb5w/oUKSGP64UPGHGOSDhVSyLs=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=TJCO7fgmcMYBDMjoQM3ARrMSsy8e0x2FQylwHlyhupvB0LwC7dxWVyCkRCUq2wpgs fxwSQ6woBiOLCl4GDpBpgoX4HbuTc27+GEuchptAj954bO2yOoVE7X0Mn7OF4GrjVJ IPL+SaSf98IBAfs5WjtPtNJ5p7trYSrdU54iD2GU= Message-ID: <2d02dd86-8524-9744-ecc1-d52d3881b3fb@linux.microsoft.com> Date: Tue, 16 Nov 2021 18:54:49 -0500 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.3.0 Subject: Re: [edk2-devel] Uncrustify configuration file and file/function templates To: Pedro Falcato Cc: edk2-devel-groups-io References: <42fc968b-0108-da06-42a4-d946f0592072@linux.microsoft.com> <6ef9d0eb-942f-943b-5a01-ca7602d31459@linux.microsoft.com> From: "Michael Kubacki" In-Reply-To: Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable The poc_5 branch config file is based on the file I'm pushing to the=20 Uncrustify fork soon:=20 https://dev.azure.com/projectmu/Uncrustify/_git/Uncrustify/pullrequest/24?_= a=3Dfiles&path=3D/etc/edk2.cfg So they're already being used and the file in the poc_5 branch was of=20 course used to generate the results in the branch. Note that I do have "align_func_params" set to true. I believe I saw=20 some occasional anomalies in the past when I set=20 "align_func_params_span" and the gap to larger values but I don't recall=20 exactly what they were. I'd be happy to try that again soon though. On 11/16/2021 6:30 PM, Pedro Falcato wrote: > Yes, I do. >=20 > Essentially, my changes make it so the function parameters are aligned=20 > and there are always two spaces between the parameter's type and name;=20 > it also adds spaces between control statements, as you mentioned. > "force" instead of "add" for sp_before_sparen is a good idea though. > I assume these are good additions to the "upstream" edk2.cfg? >=20 > On Tue, Nov 16, 2021 at 11:20 PM Michael Kubacki=20 > > wrot= e: >=20 > Hi Pedro, >=20 > By "new config", do you mean the config file in the latest branch > https://github.com/makubacki/edk2/blob/uncrustify_poc_5/uncrustify.cf= g > )? >=20 > The main difference I see from the diff in that gist and poc_5 is tha= t > "align_func_params_gap" and "align_func_params_span" are two instead = of > zero. >=20 > The value "force" instead of "add" for "sp_before_sparen" will ensure= a > single space is always added between a control statement and the > parenthesis. >=20 > Thanks, > Michael >=20 > On 11/16/2021 6:05 PM, Pedro Falcato wrote: > > Hi, > > > > RE: Uncrustify's configuration > > > > I had made a few changes to the config file, when trying out > Uncrustify, > > a few months ago, on Mike Kinney's suggestion. In my experience, > the new > > config file reflects the edk2 coding style better. > > > > In particular, it adds spaces between things like 'if' and the > > parenthesis, and better aligns functions' parameters. > > Feel free to check out the diff: > > https://gist.github.com/heatd/5bb048a188726cb8bc9f5b4a844b0ec2 > > > >. > > There's a bit of diff noise due to it being based on > > > https://dev.azure.com/projectmu/_git/Uncrustify?path=3D/etc/edk2.cfg > > > > > but >=20 > > diffed on > > > https://github.com/makubacki/edk2/blob/uncrustify_poc_2/.uncrustify/e= dk2.cfg > >=20 > > > >. > > > > Thanks, > > Pedro > > > > On Tue, Nov 16, 2021 at 7:26 PM Michael Kubacki > > > >> wrote: > > > >=C2=A0 =C2=A0 =C2=A0I think that makes sense. I will look into it f= urther and let > you know > >=C2=A0 =C2=A0 =C2=A0if there's any downsides found. > > > >=C2=A0 =C2=A0 =C2=A0On 11/16/2021 2:18 PM, Kinney, Michael D wrote: > >=C2=A0 =C2=A0 =C2=A0 > Could we add this feature to the Uncrustify = CI Plugin? > >=C2=A0 =C2=A0 =C2=A0 > > >=C2=A0 =C2=A0 =C2=A0 > Mike > >=C2=A0 =C2=A0 =C2=A0 > > >=C2=A0 =C2=A0 =C2=A0 >> -----Original Message----- > >=C2=A0 =C2=A0 =C2=A0 >> From: Michael Kubacki > >=C2=A0 =C2=A0 =C2=A0 >> > >=C2=A0 =C2=A0 =C2=A0 >> Sent: Tuesday, November 16, 2021 10:54 AM > >=C2=A0 =C2=A0 =C2=A0 >> To: devel@edk2.groups.io > >; Kinney, > >=C2=A0 =C2=A0 =C2=A0Michael D > >=C2=A0 =C2=A0 =C2=A0 >> > >=C2=A0 =C2=A0 =C2=A0 >> Subject: Re: [edk2-devel] Uncrustify config= uration file and > >=C2=A0 =C2=A0 =C2=A0file/function templates > >=C2=A0 =C2=A0 =C2=A0 >> > >=C2=A0 =C2=A0 =C2=A0 >> I would prefer to have a single version of = the file if > possible to > >=C2=A0 =C2=A0 =C2=A0 >> reduce synchronization issues across the tw= o copies. > >=C2=A0 =C2=A0 =C2=A0 >> > >=C2=A0 =C2=A0 =C2=A0 >> It seems that a CI plugin to read the conte= nts of the > template > >=C2=A0 =C2=A0 =C2=A0files and > >=C2=A0 =C2=A0 =C2=A0 >> search incoming code for that text wouldn't= be too > difficult to > >=C2=A0 =C2=A0 =C2=A0add as a > >=C2=A0 =C2=A0 =C2=A0 >> new plugin. > >=C2=A0 =C2=A0 =C2=A0 >> > >=C2=A0 =C2=A0 =C2=A0 >> Thanks, > >=C2=A0 =C2=A0 =C2=A0 >> Michael > >=C2=A0 =C2=A0 =C2=A0 >> > >=C2=A0 =C2=A0 =C2=A0 >> On 11/16/2021 1:31 PM, Michael D Kinney wro= te: > >=C2=A0 =C2=A0 =C2=A0 >>> Hi Michael, > >=C2=A0 =C2=A0 =C2=A0 >>> > >=C2=A0 =C2=A0 =C2=A0 >>> Should we have 2 versions of the config fi= le? > >=C2=A0 =C2=A0 =C2=A0 >>> > >=C2=A0 =C2=A0 =C2=A0 >>> One used by automation tools such as CI an= d git hooks > that do > >=C2=A0 =C2=A0 =C2=A0not use the > >=C2=A0 =C2=A0 =C2=A0 >>> templates. > >=C2=A0 =C2=A0 =C2=A0 >>> > >=C2=A0 =C2=A0 =C2=A0 >>> And another one that a developer can optio= nally use that > will > >=C2=A0 =C2=A0 =C2=A0add the > >=C2=A0 =C2=A0 =C2=A0 >>> templates for missing file/function header= s that the > developer > >=C2=A0 =C2=A0 =C2=A0then needs > >=C2=A0 =C2=A0 =C2=A0 >>> to fill out. > >=C2=A0 =C2=A0 =C2=A0 >>> > >=C2=A0 =C2=A0 =C2=A0 >>> One concern I have about the templates is = if they get > used but > >=C2=A0 =C2=A0 =C2=A0a developer > >=C2=A0 =C2=A0 =C2=A0 >>> does not fill in the missing information.= =C2=A0 It would be > best if > >=C2=A0 =C2=A0 =C2=A0a CI check > >=C2=A0 =C2=A0 =C2=A0 >>> rejects a file/function header that has no= t been filled in. > >=C2=A0 =C2=A0 =C2=A0 >>> > >=C2=A0 =C2=A0 =C2=A0 >>> Mike > >=C2=A0 =C2=A0 =C2=A0 >>> > >=C2=A0 =C2=A0 =C2=A0 >>>> -----Original Message----- > >=C2=A0 =C2=A0 =C2=A0 >>>> From: devel@edk2.groups.io > > > >=C2=A0 =C2=A0 =C2=A0 > >> On > Behalf Of > >=C2=A0 =C2=A0 =C2=A0Michael Kubacki > >=C2=A0 =C2=A0 =C2=A0 >>>> Sent: Tuesday, November 16, 2021 10:25 AM > >=C2=A0 =C2=A0 =C2=A0 >>>> To: devel@edk2.groups.io > >; > >=C2=A0 =C2=A0 =C2=A0Kinney, Michael D > >=C2=A0 =C2=A0 =C2=A0 >> > >=C2=A0 =C2=A0 =C2=A0 >>>> Subject: Re: [edk2-devel] Uncrustify conf= iguration file and > >=C2=A0 =C2=A0 =C2=A0file/function templates > >=C2=A0 =C2=A0 =C2=A0 >>>> > >=C2=A0 =C2=A0 =C2=A0 >>>> Hi Mike, > >=C2=A0 =C2=A0 =C2=A0 >>>> > >=C2=A0 =C2=A0 =C2=A0 >>>> Those were just disabled because I typica= lly run a separate > >=C2=A0 =C2=A0 =C2=A0invocation > >=C2=A0 =C2=A0 =C2=A0 >>>> of Uncrustify with them enabled to isolat= e code which > is missing > >=C2=A0 =C2=A0 =C2=A0 >>>> file/function headers. My thought was the= templates are > >=C2=A0 =C2=A0 =C2=A0helpful but we > >=C2=A0 =C2=A0 =C2=A0 >>>> would need to individually identify where= they are > placed to > >=C2=A0 =C2=A0 =C2=A0file TCBZs > >=C2=A0 =C2=A0 =C2=A0 >>>> for maintainers to replace the template w= ith the actual > >=C2=A0 =C2=A0 =C2=A0information. > >=C2=A0 =C2=A0 =C2=A0 >>>> > >=C2=A0 =C2=A0 =C2=A0 >>>> In some of my previous poc branches (like > >=C2=A0 =C2=A0 =C2=A0 >>>> > > > https://github.com/makubacki/edk2/commits/uncrustify_poc_3_with_heade= rs > > > =20 > =C2=A0>), > >=C2=A0 =C2=A0 =C2=A0I > >=C2=A0 =C2=A0 =C2=A0 >>>> also pushed a branch with those results. > >=C2=A0 =C2=A0 =C2=A0 >>>> > >=C2=A0 =C2=A0 =C2=A0 >>>> So I do think we would want them enabled = in the final > config > >=C2=A0 =C2=A0 =C2=A0file. We > >=C2=A0 =C2=A0 =C2=A0 >>>> can also review the contents of the templ= ates in the future > >=C2=A0 =C2=A0 =C2=A0patch series > >=C2=A0 =C2=A0 =C2=A0 >>>> to see if any changes are recommended. > >=C2=A0 =C2=A0 =C2=A0 >>>> > >=C2=A0 =C2=A0 =C2=A0 >>>> I prefer using a .uncrustify directory to= help group > related > >=C2=A0 =C2=A0 =C2=A0collateral > >=C2=A0 =C2=A0 =C2=A0 >>>> but I don't have a strong opinion there. > >=C2=A0 =C2=A0 =C2=A0 >>>> > >=C2=A0 =C2=A0 =C2=A0 >>>> Thanks, > >=C2=A0 =C2=A0 =C2=A0 >>>> Michael > >=C2=A0 =C2=A0 =C2=A0 >>>> > >=C2=A0 =C2=A0 =C2=A0 >>>> On 11/16/2021 12:16 PM, Michael D Kinney = wrote: > >=C2=A0 =C2=A0 =C2=A0 >>>>> Hi Michael, > >=C2=A0 =C2=A0 =C2=A0 >>>>> > >=C2=A0 =C2=A0 =C2=A0 >>>>> In your POC branch > >=C2=A0 =C2=A0 =C2=A0(https://github.com/makubacki/edk2/tree/uncrust= ify_poc_5 > > >=C2=A0 =C2=A0 =C2=A0 >), I see th= e > >=C2=A0 =C2=A0 =C2=A0 >>>>> uncrustify.cfg configuration file in the= root. > >=C2=A0 =C2=A0 =C2=A0 >>>>> > >=C2=A0 =C2=A0 =C2=A0 >>>>> > > > https://github.com/makubacki/edk2/blob/uncrustify_poc_5/uncrustify.cf= g > > > =20 > =C2=A0> > >=C2=A0 =C2=A0 =C2=A0 >>>>> > >=C2=A0 =C2=A0 =C2=A0 >>>>> However, in your Wiki, you provide examp= les where this > >=C2=A0 =C2=A0 =C2=A0configuration file is in an > >=C2=A0 =C2=A0 =C2=A0 >>>>> .uncrustify directory > >=C2=A0 =C2=A0 =C2=A0 >>>>> > >=C2=A0 =C2=A0 =C2=A0 >>>>> > > > https://dev.azure.com/projectmu/Uncrustify/_wiki/wikis/Uncrustify.wik= i/1/Project-Mu-(EDK-II)-Fork-Readme > > > =20 > =C2=A0= > > >=C2=A0 =C2=A0 =C2=A0 >>>>> > >=C2=A0 =C2=A0 =C2=A0 >>>>> The uncrustify.cfg files also contains c= ommented out > settings > >=C2=A0 =C2=A0 =C2=A0for the file header > >=C2=A0 =C2=A0 =C2=A0 >>>>> and function header templates. > >=C2=A0 =C2=A0 =C2=A0 >>>>> > >=C2=A0 =C2=A0 =C2=A0 >>>>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 # cmt_insert_= file_header=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =3D > >=C2=A0 =C2=A0 =C2=A0default_file_header.txt > >=C2=A0 =C2=A0 =C2=A0 >>>>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 # cmt_insert_= func_header=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =3D > >=C2=A0 =C2=A0 =C2=A0default_function_header.txt > >=C2=A0 =C2=A0 =C2=A0 >>>>> > >=C2=A0 =C2=A0 =C2=A0 >>>>> Are these disabled on purpose? > >=C2=A0 =C2=A0 =C2=A0 >>>>> > >=C2=A0 =C2=A0 =C2=A0 >>>>> Do we want to enable them?=C2=A0 If so, = should the uncrustify > >=C2=A0 =C2=A0 =C2=A0configuration file > >=C2=A0 =C2=A0 =C2=A0 >>>>> and the templates go into a .uncrustify = directory? > >=C2=A0 =C2=A0 =C2=A0 >>>>> > >=C2=A0 =C2=A0 =C2=A0 >>>>> Thanks, > >=C2=A0 =C2=A0 =C2=A0 >>>>> > >=C2=A0 =C2=A0 =C2=A0 >>>>> Mike > >=C2=A0 =C2=A0 =C2=A0 >>>>> > >=C2=A0 =C2=A0 =C2=A0 >>>>> > >=C2=A0 =C2=A0 =C2=A0 >>>>> > >=C2=A0 =C2=A0 =C2=A0 >>>>> > >=C2=A0 =C2=A0 =C2=A0 >>>>> > >=C2=A0 =C2=A0 =C2=A0 >>>>> > >=C2=A0 =C2=A0 =C2=A0 >>>>> > >=C2=A0 =C2=A0 =C2=A0 >>>> > >=C2=A0 =C2=A0 =C2=A0 >>>> > >=C2=A0 =C2=A0 =C2=A0 >>>> > >=C2=A0 =C2=A0 =C2=A0 >>>> > >=C2=A0 =C2=A0 =C2=A0 >>> > >=C2=A0 =C2=A0 =C2=A0 >>> > >=C2=A0 =C2=A0 =C2=A0 >>> > >=C2=A0 =C2=A0 =C2=A0 >>> > >=C2=A0 =C2=A0 =C2=A0 >>> > >=C2=A0 =C2=A0 =C2=A0 >>> > > > > > > > > > > > > > > > > -- > > Pedro Falcato > >=20 >=20 >=20 >=20 > --=20 > Pedro Falcato