On 11/13/2023 6:58 AM, Laszlo Ersek wrote:
Hi Michael,
recently I encountered an uncrustify failure on github.
The reason was that my local uncrustify was *more recent* (73.0.8) than
the one we use in edk2 CI (which is 73.0.3, per the edk2 file
".pytool/Plugin/UncrustifyCheck/uncrustify_ext_dep.yaml").
Updating the version number in the YAML file (i.e., advancing edk2 to
version 73.0.8) seems easy enough, but:
- Do you think 73.0.8 is mature enough for adoption in edk2?
This upstream uncrustify release was tagged in April (and I can't see
any more recent commits), so I assume it should be stable.
Yes, it is stable. We've been using each new Uncrustify release against
edk2 code in Project Mu during that time. I updated our code to include
that change in September 2022 -
https://github.com/microsoft/mu_basecore/commit/6932526bee9a2f5f3af7588923beae5e5d8fd128.
The changes since then have been additional build support for Linux and
Windows Arm and macOS.
I originally did not bring this to edk2 right away to verify stability
over time and reduce thrash if any other changes came in to consolidate
overall disruption to edk2.
- Would the version update require a whole-tree re-uncrustification?
Yes. I just did it. It is relatively minor and impacts expected code areas.
https://github.com/tianocore/edk2/pull/5043/files
I'm happy to send that to the list if desired.
The reason I'm not just ignoring this topic is that 73.0.8 actually
produces *better output* than 73.0.3, at least in the one instance I
encountered. Compare:
diff --git
a/OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.c
b/OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.c
index 434cdca84b23..3a6f75988220 100644
--- a/OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.c
+++ b/OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.c
@@ -43,12 +43,12 @@ STATIC EFI_INCOMPATIBLE_PCI_DEVICE_SUPPORT_PROTOCOL
STATIC CONST EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR mMmio64Configuration = {
ACPI_ADDRESS_SPACE_DESCRIPTOR, // Desc
(UINT16)( // Len
- sizeof
(EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR) -
- OFFSET_OF (
-
EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR,
- ResType
- )
- ),
+ sizeof (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR) -
+ OFFSET_OF (
+ EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR,
+ ResType
+ )
+ ),
ACPI_ADDRESS_SPACE_TYPE_MEM, // ResType
0, // GenFlag
0, // SpecificFlag
@@ -77,12 +77,12 @@ STATIC CONST EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR
mMmio64Configuration = {
STATIC CONST EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR mOptionRomConfiguration = {
ACPI_ADDRESS_SPACE_DESCRIPTOR, // Desc
(UINT16)( // Len
- sizeof
(EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR) -
- OFFSET_OF (
-
EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR,
- ResType
- )
- ),
+ sizeof (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR) -
+ OFFSET_OF (
+ EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR,
+ ResType
+ )
+ ),
ACPI_ADDRESS_SPACE_TYPE_MEM, // ResType
0, // GenFlag
0, // Disable option roms
SpecificFlag
Note that 73.0.3 indents the subexpression to the "//" comment on the
previous line, while 73.0.8 ignores the comment -- which I think is
justified here.
I believe this improvement may come from uncrustify commit 239c4fad745b
("Prevent endless indentation scenario in struct assignment",
2022-07-29). I think it's worth having in edk2.
CC: stewards, Pedro (commit 6ded9f50c3aa), Marcin (traditionally a big
fan of uncrustify :))
Thanks
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111177): https://edk2.groups.io/g/devel/message/111177
Mute This Topic: https://groups.io/mt/102559740/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
