On 03/17/16 11:11, Leif Lindholm wrote: > On Wed, Mar 16, 2016 at 07:06:12PM -0700, Andrew Fish wrote: >> >>> 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. > > 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?
Well, "ARG" does disturb me a bit. In this case, "parameter" is a better match I think. >From C99, 3.3p1: *argument* actual argument actual parameter (deprecated) expression in the comma-separated list bounded by the parentheses in a function call expression, or a sequence of preprocessing tokens in the comma-separated list bounded by the parentheses in a function-like macro invocation 3.15p1: *parameter* formal parameter formal argument (deprecated) object declared as part of a function declaration or definition that acquires a value on entry to the function, or an identifier from the comma-separated list bounded by the parentheses immediately following the macro name in a function-like macro definition Where we would employ this macro is function definitions (not function calls), so I would propose PARAM_UNUSED or just UNUSED, over ARG_UNUSED. Thanks Laszlo > > 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: 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
