Hi Michael, On Thu, Mar 17, 2016 at 04:29:15PM +0000, Kinney, Michael D wrote: > I am curious what value enabling the warning for an unused argument > adds. Can someone provide some examples where enabling this warning > and adding the UNUSED tag addresses real issues? I am concerned > that we are enabling a warning that would impose a large number of > source changes with little or no value. I can be convinced > otherwise, but would like to see some supporting data.
I am in no way suggesting that we turn this option on by default, even for DEBUG builds. But it _is_ valuable to be able to build with all possible warnings for the purpose of spotting undefined behaviour. The trigger for this whole exercise was a presentation at Linaro Connect last week by the toolchain guys, going into quite some details on what undefined behaviour has caused real issues in other projects (like the Linux kernel) after GCC started becoming more aggressive in its optimisations around 4.8 timeframe. If you want to develop difficulties sleeping, have a look at http://connect.linaro.org/resource/bkk16/bkk16-503/ > Also, what are the requirements for maintenance of code once if this > new UNUSED macro is used? We have function implementations where an > argument may not be used today, but a bug fix/feature enhancement > later may use the UNUSED argument. Do we get a different warning > because it was tagged as UNUSED, but it is actually used now? Or is > that silent? All the UNUSED argument does is instruct the compiler to not give off the warning when encountering it whilst building with the warning enabled. > I think there are a few categories of functions to consider here: > > 1) Protocol/PPI functions. The parameters for functions are defined > for a wide usage sometimes with optional behavior. Specific > implementations may choose to not implement some optional > behavior, and hence may not require some of the parameters. I do > not think a warning or an error should be generated by a > conformant implementation that does not use a parameter. > > 2) Library Class functions. The parameters for functions are also > defined for a wide usage sometimes with optional behavior. > Specific implementations may choose to not implement optional > behavior, and hence may not require some of the parameters. I do > not think a warning or an error should be generated by a specific > library instance that does not use a parameter in a public > library class API. > > 3) Internal functions. If an internal function does not use a > parameter, then remove the parameter from the function call. > > Can we enable /Wall and disable this specific warning? Or is there > a concern that category (3) cannot be detected without enabling this > warning? Maybe it would be better if we could find a way to disable > this warning for all APIs that are decorated with EFIAPI? Again, the intent is not to have this warning (or in my case -Weverything) enabled in tools_def.template. At least not for a very long time, until we've reached a place where doing so would not be expected to trigger build failures. Basically, -Wall is too week and catches too few errors, but the GCC folks have conceded that if they extend its scope, much existing software stops building. They did sound keen to add a -Weverything option to GCC as well, though. And working through and attempting to build with -Weverything _has_ picked up a fair amount of real bugs, so I think this is worthwhile. Regards, Leif > > Thanks, > > Mike > > > -----Original Message----- > > From: Leif Lindholm [mailto:[email protected]] > > Sent: Thursday, March 17, 2016 8:53 AM > > To: [email protected] > > Cc: Kinney, Michael D <[email protected]>; Gao, Liming > > <[email protected]> > > Subject: [PATCH] MdePkg: add UNUSED notation to Base.h > > > > To permit many existing platforms to build with -Wunused-parameter, on > > GCC and CLANG, the unused parameters need to be annotated as such. > > Existing regexp code already uses ARG_UNUSED for this, but it is really > > needed across the codebase - so add a version called UNUSED in Base.h. > > > > Contributed-under: TianoCore Contribution Agreement 1.0 > > Signed-off-by: Leif Lindholm <[email protected]> > > --- > > > > Change since RFC: rename ARG_UNUSED -> UNUSED. > > > > MdePkg/Include/Base.h | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/MdePkg/Include/Base.h b/MdePkg/Include/Base.h > > index 89b2aed..a68b209 100644 > > --- a/MdePkg/Include/Base.h > > +++ b/MdePkg/Include/Base.h > > @@ -189,6 +189,15 @@ struct _LIST_ENTRY { > > /// > > #define OPTIONAL > > > > +/// > > +/// Function argument intentionally unused in function. > > +/// > > +#if defined(__GNUC__) || defined(__clang__) > > + #define UNUSED __attribute__ ((unused)) > > +#else > > + #define UNUSED > > +#endif > > + > > // > > // UEFI specification claims 1 and 0. We are concerned about the > > // complier portability so we did it this way. > > -- > > 2.1.4 > _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
