Re: [edk2-devel] [RFC PATCH] ArmPkg: Enable AuditMode for Uncrustify CI checks

["Michael Kubacki" <mikuback@linux.microsoft.com>]

Sorry, this message was accidentally sent while I was adjusting 
formatting. I reply to the original mail with a clean reply.

On 6/2/2023 11:18 AM, Michael Kubacki wrote:
> Hi Ard,
> 
> Are there particular areas that could be improved to make it more usable 
> for you? I'm trying to find actionable improvements that can be made, if 
> any.
> 
> I know it's not perfect but developers can run it with a single keyboard 
> shortcut and it's been useful internally for eliminating style minutia 
> from distracting design and correctness conversation in code reviews.
> 
> 
> 
> 
>  > - finding out from the web UI what exactly Uncrustify objected to is not
>  >    straight-forward.
>  >
> This should not be necessary. It can be run locally to produce the same 
> result as in CI.
> 
> IDE-specific, but a lot of people use VS Code with the Uncrustify 
> extension 
> (https://marketplace.visualstudio.com/items?itemName=zachflower.uncrustify) and that allows Uncrustify to be run against the open source file by simply pressing the "code formatter" command.
> 
> I usually write the code stage or commit it and then run this command on 
> the file. The code is formatted and it gives a normal local diff of 
> exactly what Uncrustify changed.
> 
> 
> On 6/2/2023 4:51 AM, Ard Biesheuvel wrote:
>> Uncrustify checks are too rigid, making them counter-productive:
>>
>> - it leads to code that is arguably harder to parse visually (e.g.,
>>    the changes to ArmPkg/Include/Chipset/AArch64Mmu.h in commit
>>    429309e0c6b74792)
> 
> Looking at commit 7f198321eec0f520373, I see positive changes like in 
> ArmCrashDumpDxe.c spacing in the ASSERT calls and treatment of 
> multi-line parameter formatting in the mCpu->RegisterInterruptHandler() 
> call are consistent.
> 
> That carries on for a number of C calls with inconsistent spacing before 
> opening parentheses and calls such as those to the DEBUG macro which I 
> find easier to read separating each parameter on a dedicated line.
> 
> In AArch64Mmu.h, I agree that preserving the (mostly) global column as 
> opposed to block-specific columns would be easier to vertically scan. Is 
> that the main issue in the file?
> 
>> - it forces indentation-only changes to code in the vicinity of actual
>>    changes, making the code history more bloated than necessary (see
>>    commit 7f198321eec0f520373 for an example)
> 
>> - finding out from the web UI what exactly Uncrustify objected to is not
>>    straight-forward.
>>
>> So let's enable AuditMode for ArmPkg, so that interested parties can see
>> the uncrustify recommendations if desired, but without preventing the
>> changes from being merged. This leaves it at the discretion of the
>> ArmPkg maintainers to decide which level of conformance is required.
>>
>> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
>> Cc: "Kinney, Michael D" <michael.d.kinney@intel.com>
>> Cc: Michael Kubacki <mikuback@linux.microsoft.com>
>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>> ---
>>   ArmPkg/ArmPkg.ci.yaml | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/ArmPkg/ArmPkg.ci.yaml b/ArmPkg/ArmPkg.ci.yaml
>> index 24db7425051388cf..d3124816118944cb 100644
>> --- a/ArmPkg/ArmPkg.ci.yaml
>> +++ b/ArmPkg/ArmPkg.ci.yaml
>> @@ -239,5 +239,10 @@
>>           ],
>>
>>           "AdditionalIncludePaths": [] # Additional paths to spell check
>>
>>                                        # (wildcards supported)
>>
>> +    },
>>
>> +
>>
>> +    # options defined in .pytool/Plugin/UncrustifyCheck
>>
>> +    "UncrustifyCheck": {
>>
>> +        "AuditOnly": True
>>
>>       }
>>
>>   }
>>
> 
> 
> 
>