On 18 March 2016 at 17:22, Laszlo Ersek <[email protected]> wrote: > On 03/18/16 15:48, Gao, Liming wrote: >> Laszlo: >> >> I understand the request to BaseTools is that AutoGen code can pass >> GCC or MSFT compiler without any warning. If so, EDKII module can enable >> non default warnings in itself. I will evaluate BaseTools. > > Thank you! > >> And, you raise one issue that the different ARCHs have the different >> warning setting. I suggest to keep them consistent to avoid the >> different build results. How about adding -Wno-unused-but-set-variable >> option to AARCH64 DEBUG. > > Can we move in the other direction perhaps? Can we remove > -Wno-unused-but-set-variable from the following settings: > > - GCC46_IA32_CC_FLAGS > - GCC46_X64_CC_FLAGS > > (these are inherited by the >= 4.6 GCC settings for IA32 and X64,) > > - RELEASE_GCC46_ARM_CC_FLAGS > - RELEASE_GCC47_ARM_CC_FLAGS > - RELEASE_GCC47_AARCH64_CC_FLAGS > - RELEASE_GCC48_ARM_CC_FLAGS > - RELEASE_GCC48_AARCH64_CC_FLAGS > - RELEASE_GCC49_ARM_CC_FLAGS > - RELEASE_GCC49_AARCH64_CC_FLAGS > > I believe all of the above settings include > "-Wno-unused-but-set-variable" only for the following reason: > > When GCC46 was enabled in BaseTools, the "-Wall" option (which we do > enable as a basis) suddenly started including > "-Wunused-but-set-variable" too. Namely, that warning option was new in > the gcc-4.6 release, and "-Wall" by default included it: > > https://gcc.gnu.org/gcc-4.6/changes.html > > The edk2 tree wouldn't build with "-Wunused-but-set-variable"; so, in > order to expedite GCC46 enablement, the "-Wno-unused-but-set-variable" > was set. See commit 2bcc713e74b94. > > However, in reality, the number of warnings triggered by this setting is > very low, in my testing -- just a handful. I listed them yesterday; the > following modules are affected (while building ArmVirtPkg and OvmfPkg): > > - MdeModulePkg/Bus/Pci/PciHostBridgeDxe > - UefiCpuPkg/Library/MtrrLib > - UefiCpuPkg/PiSmmCpuDxeSmm > > If I send a series that cleans up these warnings, would you agree (and > perhaps even test with your own platforms) to remove > "-Wno-unused-but-set-variable"? > > I agree that we should unify the warning flags, but I think we should > move towards stricter warnings, not laxer warnings -- provided we can > adapt the tree more or less easily. >
I strongly agree with Laszlo here. The issues that were caught by this warning are all examples of where refactoring of code results in variables to be left behind that are completely unused. The EDK2 codebase is difficult enough to understand as it is, and having variables with no purpose left behind only makes that worse. And indeed, since some platforms already use this flag, the codebase is mostly clean with respect to such occurrences, and could easily be kept that way. Thanks, Ard. _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
