> On Mar 16, 2016, at 7:02 PM, Gao, Liming <liming....@intel.com> 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. > >>> 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: af...@apple.com <mailto:af...@apple.com> [mailto:af...@apple.com > <mailto:af...@apple.com>] > Sent: Thursday, March 17, 2016 1:12 AM > To: Laszlo Ersek <ler...@redhat.com <mailto:ler...@redhat.com>> > Cc: Kinney, Michael D <michael.d.kin...@intel.com > <mailto:michael.d.kin...@intel.com>>; edk2-de...@ml01.01.org > <mailto:edk2-de...@ml01.01.org>; Gao, Liming <liming....@intel.com > <mailto:liming....@intel.com>>; Leif Lindholm <leif.lindh...@linaro.org > <mailto:leif.lindh...@linaro.org>> > 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 > >>> edk2-devel@lists.01.org <mailto:edk2-devel@lists.01.org> > >>> https://lists.01.org/mailman/listinfo/edk2-devel > >>> <https://lists.01.org/mailman/listinfo/edk2-devel> > >> > > > > _______________________________________________ > > edk2-devel mailing list > > edk2-devel@lists.01.org <mailto:edk2-devel@lists.01.org> > > https://lists.01.org/mailman/listinfo/edk2-devel > > <https://lists.01.org/mailman/listinfo/edk2-devel> _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
