[edk2-devel] [RFC PATCH] ArmPkg: Enable AuditMode for Uncrustify CI checks
Michael Kubacki
mikuback at linux.microsoft.com
Fri Jun 2 15:26:49 UTC 2023
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 at quicinc.com>
> Cc: "Kinney, Michael D" <michael.d.kinney at intel.com>
> Cc: Michael Kubacki <mikuback at linux.microsoft.com>
> Signed-off-by: Ard Biesheuvel <ardb at 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
>
> }
>
> }
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105659): https://edk2.groups.io/g/devel/message/105659
Mute This Topic: https://groups.io/mt/99282612/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list