Andrew, I agree that adding UNUSED to Base.h solves the issues that multiple packages may want to enable that warning and have a way to decorate implementation so the warning is not generated.
I did mix two topics in this discussion. One is code review of UNUSED macro, and the other is an EDK II C Coding Style topic. As long as use of UNUSED is optional, and each package/module owner gets to decide if use of UNUSED is required or not, then I am ok with adding the macro to Base.h. We may want to add comments above the UNUSED macro in Base.h to provide a more detailed description that describes that it is optional and if/when it should be used. We will also need to decide if we want any statements in the EDK II C Coding Style document about use of the UNUSED macro, but that is likely its own thread. Mike > -----Original Message----- > From: [email protected] [mailto:[email protected]] > Sent: Thursday, March 17, 2016 2:03 PM > To: Laszlo Ersek <[email protected]> > Cc: Leif Lindholm <[email protected]>; Kinney, Michael D > <[email protected]>; [email protected] > <[email protected]>; Qiu, > Shumin <[email protected]>; Gao, Liming <[email protected]>; Ard > Biesheuvel > <[email protected]> > Subject: Re: [edk2] [PATCH] MdePkg: add UNUSED notation to Base.h > > > > On Mar 17, 2016, at 12:57 PM, Laszlo Ersek <[email protected]> wrote: > > > > On 03/17/16 20:51, Laszlo Ersek wrote: > >> (adding Ard and Shumin because the below will tie in with another thread) > >> > >> On 03/17/16 19:05, Leif Lindholm wrote: > >> > >>> I must confess to no small amount of surprise that optionally adding > >>> the ability to tag an unused argument as unused is controversial. > > > >> If I understand correctly, if we wanted to enable "-Wunused-parameter > >> -Wunused-but-set-parameter" even just occasionally, these ~4000 > >> instances would have to be audited, and each should be either fixed > >> (i.e., internal functions should drop the parameters) or marked UNUSED > >> (i.e., library instances and PPI/protocol implementations should > >> annotate their definitions of public functions). > >> > >> Thus, this is what surprises me. It looks daunting. > > > > Small clarification: if you'd like to selectively add > > "-Wunused-parameter -Wunused-but-set-parameter" to the [BuildOptions] of > > a number of (non-core?) modules (in their INF files), and employ UNUSED > > in connection with that, I certainly think that's a valid use case. > > > > To me it does justify this patch. Namely, perhaps marking parameters as > > UNUSED will not be enforced across the entire tree, but if a module > > owner would like to enable those warnings on his/her turf, then he/she > > should be able to annotate the unused parameters with a macro that is > > centrally defined. The macro definition should be universal, even though > > its application might not be. > > > > That sounds reasonable to me. > > I think in general you will find that Mike Kinney and I are for turning on > every > compiler warning that is practical to use. > > Thanks, > > Andrew Fish > > > Thanks > > Laszlo > > > > _______________________________________________ > > 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
