Re: [edk2-devel] Progress on getting Uncrustify working for EDK2?

["Andrew Fish" <afish@apple.com>]

[-- Attachment #1: Type: text/plain, Size: 6129 bytes --]



> On Oct 7, 2021, at 6:48 AM, Leif Lindholm <leif@nuviainc.com> wrote:
> 
> Hi Michael,
> 
> Apologies, I've owed you a response (promised off-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 constrained to:
> - Newline after '(' for multi-line function calls.
> - Dealing with "(("/"))" for DEBUG macros.
> - Function pointer typedefs:
>  - typedef\nEFIAPI
>  - closing parentheses indentation
> 
> 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.
> 
> Taking these in order:
> 
> 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.
> 
> 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?

Can we disable the rule that does not like the DEBUG macro? I seem to remember clang nags about some strange () usage, so maybe we would not lose that much?

Thanks,

Andrew Fish

> 
> 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?
> 
> Best Regards,
> 
> 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 the 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 experiment 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 should 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 than in the examples in those branches. For
>> example, remove heuristics that when run against the same set of code
>> multiple times can produce different results. An example would be a rule
>> that reformats code because it exceeds a specified column width on one 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 might 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 code as
>> an example and turn on the plugin. The CI plugin runs Uncrustify against
>> 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.
>> 
>> 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.
>> 
>> Regarding indentation, Marvin is right that Uncrustify cannot support 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 massively
>> deploy changes across the codebase in case a decision were made to change
>> the style.
>> 
>> Thanks,
>> Michael
>> 
>> On 8/16/2021 3:39 PM, Marvin Häuser wrote:
>>> Hey Rebecca,
>>> 
>>> I think even Uncrustify has issues with the EDK II indentation style.
>>> You might want to check the UEFI Talkbox Discord server, I had a brief
>>> chat with Michael about it there. I don't think realistically any tool
>>> supports EDK II's indentation 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 -> LF
>>> change does that anyway, so at least two evils could be dealt with in
>>> one go really.
>>> 
>>> Best regards,
>>> 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 Uncrustify
>>>>> for EDK2 instead of the ECC tool, and came across https://www.mail-archive.com/search?l=devel@edk2.groups.io&q=subject:%22Re%5C%3A+%5C%5Bedk2%5C-devel%5C%5D+TianoCore+Community+Meeting+Minutes+%5C-+2%5C%2F4%22&o=newest&f=1
>>>>> .
>>>>> 
>>>>> 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 that we can use as a reference
>>>>> for a conversation around uncrustify 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
>>>>> 
>>>> 
>>> 
>>> 
>>> 
>>> 
>>> 
>> 
>> 
>> 
>> 
>> 
>> 
> 
> 
> 


[-- Attachment #2: Type: text/html, Size: 41425 bytes --]