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

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

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.

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)

That's true. People adjusted things like indentation before depending on 
a given change, but it is predetermined now. Perhaps turning off column 
alignment (in general or in a given package) could help reduce noise 
from this.

I don't think alignment enforcement is necessary. That might also help 
address some of thrash in AArch64Mmu.h.

> - 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.

> 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
> 
>       }
> 
>   }
>