> On Mar 17, 2016, at 9:29 AM, Kinney, Michael D <[email protected]> > wrote: > > Hello, > > 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. >
Mike, It looks like the idea comes from: MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regenc.h #ifndef ARG_UNUSED #if defined(__GNUC__) # define ARG_UNUSED __attribute__ ((unused)) #else # define ARG_UNUSED #endif #endif > 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? > > 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. > I noticed the same thing. Enforcing this would be a lot of work in the edk2 code base. > Can we enable /Wall and disable this specific warning? This is what we do for clang. Given how Protocols and PPIs are defined I think you have to have these compiler settings today to compile the edk2. > 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? > I think the point of __attribute__ ((unused)) is to suppress the warning if it is enabled. Since we suppress the diagnostic from the compiler we don't need this. Thanks, Andrew Fish > 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 _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
