Audit extraneous parentheses

[mikebeaton]

I was struggling to get much response to this small PR, possibly because it is a piecemeal update adding a change which ideally would have been included in this larger merged PR. And, one might reasonably guess, there will be other instances around too - which have not yet been hit by builds I have tested.

The problem is extraneous parentheses around comparisons, e.g. if ((a == 1)) { ... }, which give a build warning error on the XCODE5 toolchain.

I manually scanned for possible instances of this using various chained greps. After manually removing non-instances (it's hard to match all and only this), I found 54 instances of this pattern (I think this is probably all single-line instances, though since the scan was done by hand, I can't guarantee it), and additionally several other linting issues in similarly patterned lines, as below.

I believe these are all genuine linting isses, and all worth fixing.

Would a PR fixing these be welcome? (For the main issue, would a PR updating Uncrustify to address this be preferable?)

//
// Excess space after !=/==
//
./BaseTools/Source/C/GenFw/Elf32Convert.c:              if ((gMovwOffset + 4) !=  (mCoffSectionsOffset[RelShdr->sh_info] + (Rel->r_offset - SecShdr->sh_addr))) {
./MdeModulePkg/Universal/HiiDatabaseDxe/Font.c:  if ((Flags & (EFI_HII_OUT_FLAG_WRAP | EFI_HII_OUT_FLAG_CLIP_CLEAN_X)) ==  (EFI_HII_OUT_FLAG_WRAP | EFI_HII_OUT_FLAG_CLIP_CLEAN_X)) {

//
// Excess parentheses around item which is cast
//
./BaseTools/Source/C/GenFw/Elf32Convert.c:    if ((*Filter)(Shdr)) {
./BaseTools/Source/C/GenFw/Elf64Convert.c:    if ((*Filter)(Shdr)) {

//
// & should be &&
//
./ArmPkg/Library/ArmDisassemblerLib/ThumbDisassembler.c:          if ((Rn == 0xf) & (AsciiStrCmp (gOpThumb2[Index].Start, "BFC") == 0)) {

//
// Should probably have != 0 added; in which case there  are six other similar instances in EmulatorPkg which should be changed
//
./EmulatorPkg/Win/Host/WinFileSystem.c:    if ((OpenMode & EFI_FILE_MODE_CREATE)) {
./EmulatorPkg/Unix/Host/PosixFileSystem.c:    if ((OpenMode & EFI_FILE_MODE_CREATE)) {

//
// The remainder are straightforward 'equality comparison with extraneous parentheses'; some, but not all, complain if their respective packages are built on XCODE5
//
./PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c:        if ((CompareHMS (From, To) >= 0)) {
./PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c:        if ((CompareHMS (From, To) <= 0)) {
./PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c:          if ((CompareHMS (From, To) >= 0)) {
./PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c:        if ((CompareHMS (From, To) >= 0)) {
./PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c:    if ((CompareHMS (From, To) >= 0)) {
./NetworkPkg/IScsiDxe/IScsiConfig.c:      if ((Gateway.Addr[0] != 0)) {
./NetworkPkg/Mtftp6Dxe/Mtftp6Option.c:      if ((Value < 1)) {
./BaseTools/Source/C/GenFw/GenFw.c:    if ((stricmp (argv[0], "--rebase") == 0)) {
./BaseTools/Source/C/GenFw/GenFw.c:    if ((stricmp (argv[0], "--address") == 0)) {
./BaseTools/Source/C/GenFw/GenFw.c:    if ((PeHdr->Pe32.OptionalHeader.SectionAlignment != PeHdr->Pe32.OptionalHeader.FileAlignment)) {
./BaseTools/Source/C/GenFv/GenFvInternalLib.c:        if ((ImgHdr->Pe32.OptionalHeader.SectionAlignment != ImgHdr->Pe32.OptionalHeader.FileAlignment)) {
./BaseTools/Source/C/GenFv/GenFvInternalLib.c:        if ((ImgHdr->Pe32.OptionalHeader.SectionAlignment != ImgHdr->Pe32.OptionalHeader.FileAlignment)) {
./BaseTools/Source/C/VolInfo/VolInfo.c:    if ((stricmp (argv[0], "--hash") == 0)) {
./BaseTools/Source/C/TianoCompress/TianoCompress.c:  if ((strcmp(argv[1], "--version") == 0)) {
./BaseTools/Source/C/VfrCompile/VfrUtilityLib.cpp:    if ((VarGuid != NULL)) {
./BaseTools/Source/C/EfiRom/EfiRom.c:  if ((stricmp(Argv[0], "--version") == 0)) {
./OvmfPkg/PlatformPei/AmdSev.c:  if ((Status != 0)) {
./UefiCpuPkg/PiSmmCpuDxeSmm/NonMmramMapDxeSmm.c:    if ((MemorySpaceMap[Index].GcdMemoryType == EfiGcdMemoryTypeMemoryMappedIo)) {
./IntelFsp2WrapperPkg/Library/PeiFspWrapperApiTestLib/FspWrapperApiTest.c:    if ((CompareGuid (&Hob.ResourceDescriptor->Owner, &gFspBootLoaderTolumHobGuid))) {
./MdePkg/Library/BasePeCoffLib/BasePeCoff.c:    if ((NumberOfRvaAndSizes < EFI_IMAGE_DIRECTORY_ENTRY_BASERELOC)) {
./RedfishPkg/RedfishHttpDxe/RedfishHttpDxe.c:    if ((Response->StatusCode != NULL)) {
./RedfishPkg/RedfishHttpDxe/RedfishHttpDxe.c:    if ((Response->StatusCode != NULL)) {
./RedfishPkg/RedfishHttpDxe/RedfishHttpDxe.c:    if ((Response->StatusCode != NULL)) {
./RedfishPkg/RedfishHttpDxe/RedfishHttpDxe.c:    if ((Response->StatusCode != NULL)) {
./RedfishPkg/RedfishHttpDxe/RedfishHttpDxe.c:    if ((Response->StatusCode != NULL)) {
./RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c:        if ((mRequiredProtocol[Index].ProtocolType == ProtocolTypeRestEx)) {
./ShellPkg/Library/UefiShellDebug1CommandsLib/EditMenuBar.c:    if (((Col + Width) > LastCol)) {
./ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/BufferImage.c:  if ((Column % 3 == 2)) {
./ShellPkg/Application/Shell/ShellProtocol.c:  if ((PcdGet8 (PcdShellSupportLevel) < 1)) {
./SecurityPkg/DeviceSecurity/SpdmSecurityLib/SpdmAuthentication.c:    } else if ((LIBSPDM_STATUS_IS_ERROR (SpdmReturn))) {
./SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c:  if ((HashAlg >= HASHALG_MAX)) {
./SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c:  if ((HashAlg >= HASHALG_MAX)) {
./IntelFsp2Pkg/FspSecCore/SecFspApiChk.c:    if ((ApiIdx != FspMemoryInitApiIndex)) {
./MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c:    if ((CompareMem (Data, mMorLockKey, MOR_LOCK_V2_KEY_SIZE) == 0)) {
./MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c:    if ((GetNextGuidHob (&gEfiAuthenticatedVariableGuid, GET_NEXT_HOB (GuidHob)) != NULL)) {
./MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c:      if ((GetNextGuidHob (&gEfiVariableGuid, GET_NEXT_HOB (GuidHob)) != NULL)) {
./MdeModulePkg/Universal/Variable/Pei/Variable.c:    if ((GetNextGuidHob (&gEfiAuthenticatedVariableGuid, GET_NEXT_HOB (GuidHob)) != NULL)) {
./MdeModulePkg/Universal/Variable/Pei/Variable.c:      if ((GetNextGuidHob (&gEfiVariableGuid, GET_NEXT_HOB (GuidHob)) != NULL)) {
./MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWrite.c:  if ((Record->SpareComplete != FTW_VALID_STATE)) {
./MdeModulePkg/Universal/PCD/Dxe/Service.c:      if ((TokenNumber + 1 <= mPeiNexTokenCount + 1)) {
./MdeModulePkg/Universal/HiiDatabaseDxe/ConfigRouting.c:    if (((VarOffset + VarWidth) > VarStorageData->Size)) {
./MdeModulePkg/Universal/HiiDatabaseDxe/ConfigKeywordHandler.c:      if ((CompareMem (DevicePath, CurrentDevicePath, DevicePathSize) == 0)) {
./MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c:  } else if ((Trb != NULL)) {
./MdeModulePkg/Bus/Usb/UsbNetwork/NetworkCommon/PxeFunction.c:        if ((Cdb->DBsize != 0)) {
./MdeModulePkg/Bus/Usb/UsbNetwork/NetworkCommon/PxeFunction.c:        if ((Cdb->CPBsize == 0)) {
./MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c:        if ((NETWORK_DEVICE_LIST_FORM_ID == NextShowFormId)) {
./MdeModulePkg/Library/BaseVariableFlashInfoLib/BaseVariableFlashInfoLib.c:    if ((GetNextGuidHob (&gVariableFlashInfoHobGuid, GET_NEXT_HOB (GuidHob)) != NULL)) {
./DynamicTablesPkg/Library/Acpi/Common/AcpiPcctLib/PcctGenerator.c:  if ((PccCmObj->PlatIrq.Interrupt != 0)) {
./DynamicTablesPkg/Library/Acpi/Common/AcpiPcctLib/PcctGenerator.c:  if ((PccCmObj->PlatIrq.Interrupt != 0)) {
./DynamicTablesPkg/Library/Acpi/Common/AcpiPcctLib/PcctGenerator.c:  } else if ((PccCmObj->PlatIrq.Interrupt != 0)) {
./DynamicTablesPkg/Library/Common/AmlLib/Tree/AmlNode.c:  if ((RootNode->SdtHeader != NULL)) {
./DynamicTablesPkg/Library/Common/AmlLib/AmlEncoding/Aml.c:    if ((mAmlFieldEncoding[Index].OpCode == OpCode)) {
./DynamicTablesPkg/Library/Common/AmlLib/AmlEncoding/Aml.c:  if ((Length < (1 << 6))) {
./DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlResourceDataCodeGen.c:    if ((!AmlNodeHasOpCode (ParentNode, AML_NAME_OP, 0))) {

There is a question as to whether this would be better addressed by an update to Uncrustify. I haven't touched that code, and if anybody was able to give me a pointer as to where to get started, to add/modify a rule so that it traps the above and converts to if (a == 1) { ... }, it would be appreciated; if not, I'm sure I can get there in due course. It would be interesting to hear whether this approach would be welcome/preferred.

[mdkinney]

I think this same error is generated by CLANG as well. Right?

[mikebeaton]

No, I don't get the issue when building with CLANG, only when building with XCODE5.