On Wed, Mar 16, 2016 at 07:06:12PM -0700, Andrew Fish wrote: > > > On Mar 16, 2016, at 7:02 PM, Gao, Liming <[email protected]> wrote: > > > > Andrew: > > If we enable -Wunused-parameter option for GCC and CLANG, it will bring > > the big change to edk2 project. This patch is just to add ARG_UNUSED > > notation to Base.h. It has no impact. So, I think this patch is fine. > > > > Liming, > > I don't really have an issue adding it for source compatibility with other > projects. > > The comments from Leif imply an across the codebase change? I was > trying to point out what a large undertaking that would be.
Well, it doesn't have to be an all in one go type thing. As long as the modifier exists, it can be added where people come across problems when building with lots of warnings enabled. But a follow-up question (for everyone) - is ARG_UNUSED the keyword to use, or should it just be UNUSED? Regards, Leif > > >>> On 03/14/16 16:38, Leif Lindholm wrote: > > >>>> 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 in Base.h. > > > Thanks, > > Andrew Fish > > > > Thanks > > Liming <> > > <>From: [email protected] <mailto:[email protected]> [mailto:[email protected] > > <mailto:[email protected]>] > > Sent: Thursday, March 17, 2016 1:12 AM > > To: Laszlo Ersek <[email protected] <mailto:[email protected]>> > > Cc: Kinney, Michael D <[email protected] > > <mailto:[email protected]>>; [email protected] > > <mailto:[email protected]>; Gao, Liming <[email protected] > > <mailto:[email protected]>>; Leif Lindholm <[email protected] > > <mailto:[email protected]>> > > Subject: Re: [edk2] [RFC] MdePkg: add ARG_UNUSED notation to Base.h > > > > > > > On Mar 16, 2016, at 10:06 AM, Laszlo Ersek wrote: > > > > > > On 03/16/16 17:50, Andrew Fish wrote: > > >> > > >>> On Mar 16, 2016, at 6:54 AM, Laszlo Ersek wrote: > > >>> > > >>> On 03/14/16 16:38, Leif Lindholm wrote: > > >>>> 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 in Base.h. > > >>>> > > >>>> Contributed-under: TianoCore Contribution Agreement 1.0 > > >>>> Signed-off-by: Leif Lindholm > > >>>> --- > > >>>> > > >>>> This is really the result of a friendly toolchain engineer informing me > > >>>> CLANG has the -Weverything flag, to actually enable all possible > > >>>> warnings. > > >>>> > > >>>> One problem trying to pick out the real bugs from the just not entirely > > >>>> clear code is that basically a lot of *LibNull implementations, and > > >>>> some libraries that should be usable, trip build failures due to unused > > >>>> parameters. > > >>>> > > >>>> MdePkg/Include/Base.h | 9 +++++++++ > > >>>> 1 file changed, 9 insertions(+) > > >>>> > > >>>> diff --git a/MdePkg/Include/Base.h b/MdePkg/Include/Base.h > > >>>> index 89b2aed..f789094 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 ARG_UNUSED __attribute__ ((unused)) > > >>>> +#else > > >>>> + #define ARG_UNUSED > > >>>> +#endif > > >>>> + > > >>>> // > > >>>> // UEFI specification claims 1 and 0. We are concerned about the > > >>>> // complier portability so we did it this way. > > >>>> > > >>> > > >>> Support for this seems to go back to gcc-4.4: > > >>> > > >>> https://gcc.gnu.org/onlinedocs/gcc-4.4.7/gcc/Variable-Attributes.html > > >>> <https://gcc.gnu.org/onlinedocs/gcc-4.4.7/gcc/Variable-Attributes.html> > > >>> > > >>> So it looks safe. (And I agree with the idea.) > > >>> > > >> > > >> I'm confused by the usage, or really when the compiler detects the > > >> warning? > > >> > > >> For example how is this going to work on public interfaces? > > >> Protocols, PPIs, or library class definitions? There is a single > > >> definition of the function, but multiple implementations. Some > > >> implementations may not use some arguments. But what arguments get > > >> used seems to be an implementation choice and not part of the API? > > >> Thus it seems like this macro is not going to enable changing > > >> compiler flags? > > > > > > I think this attribute would be used in library instances and protocol > > > implementations. It would not be used in library class headers nor > > > protocol definitions. > > > > > > Example: > > > > > > /* included from library class hdr of protocol def hdr */ > > > int f(int x); > > > > > > /* code in library instance or protocol impl */ > > > int f(int x __attribute__ ((unused))) > > > { > > > return 0; > > > } > > > > > > int main(void) > > > { > > > return f(-2); > > > } > > > > > > Compiles silently with > > > > > > $ gcc -Wall -Wextra -ansi -pedantic -Werror -o x x.c > > > > > > If I remove __attribute__ ((unused)), I get: > > > > > > x.c:5:15: error: unused parameter 'x' [-Werror=unused-parameter] > > > int f(int x) > > > ^ > > > cc1: all warnings being treated as errors > > > > > > > OK that makes sense. That is feels like it is going to be a very very large > > change to the codebase given almost every protocol and PPI member pass the > > "This" pointer, but may not use it in code. > > > > Do we need something for VC++? > > > > Thanks, > > > > Andrew Fish > > > > > Thanks > > > Laszlo > > > > > >> > > >> Or am I just confused? > > >> > > >> Thanks, > > >> > > >> Andrew Fish > > >> > > >> > > >>> Acked-by: Laszlo Ersek > > >>> _______________________________________________ > > >>> edk2-devel mailing list > > >>> [email protected] <mailto:[email protected]> > > >>> https://lists.01.org/mailman/listinfo/edk2-devel > > >>> <https://lists.01.org/mailman/listinfo/edk2-devel> > > >> > > > > > > _______________________________________________ > > > edk2-devel mailing list > > > [email protected] <mailto:[email protected]> > > > https://lists.01.org/mailman/listinfo/edk2-devel > > > <https://lists.01.org/mailman/listinfo/edk2-devel> _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
