Re: [PATCH 2/3] vec: Silence -Wunused-function warnings on clang

[Simon Marchi <simon.marchi@polymtl.ca>]

On 2017-06-26 11:28, Pedro Alves wrote:
> On 06/25/2017 06:45 PM, Simon Marchi wrote:
>> clang has a too aggressive (or broken, depends on how you want to see
>> it) -Wunused-function warning,
> 
> There's no way to avoid the warning in this use case, so I can't see
> how to call it anything but the latter.
> 
>> which is triggered by the functions
>> defined by DEF_VEC_* but not used in the current source file.  
>> Normally,
>> it won't warn about unused static inline functions defined in header
>> files, because it's expected that a source file won't use all 
>> functions
>> defined in a header file it includes.  However, the DEF_VEC_* macros
>> define those functions in source files, which leads clang to think 
>> that
>> we should remove those functions.  It is therefore missing a check to
>> see whether those functions are resulting from macro expansion.  A bug
>> already exists for that:
>> 
>>   https://bugs.llvm.org//show_bug.cgi?id=22712
>> 
>> It's quite easy to silence this warning in a localized way, that is in
>> the DEF_VEC_* macros.
>> 
>> gdb/ChangeLog:
>> 
>> 	* common/diagnostics.h (DIAGNOSTIC_IGNORE_UNUSED_FUNCTION): New
>> 	macro.
>> 	* common/vec.h: Include diagnostics.h.
>> 	(DEF_VEC_I, DEF_VEC_P, DEF_VEC_O): Ignore -Wunused-function
>> 	warning.
>> ---
>>  gdb/common/diagnostics.h |  3 +++
>>  gdb/common/vec.h         | 11 +++++++++++
>>  2 files changed, 14 insertions(+)
>> 
>> diff --git a/gdb/common/diagnostics.h b/gdb/common/diagnostics.h
>> index 35bacf2..ee824a3 100644
>> --- a/gdb/common/diagnostics.h
>> +++ b/gdb/common/diagnostics.h
>> @@ -35,9 +35,12 @@
>>  # define DIAGNOSTIC_IGNORE_SELF_MOVE DIAGNOSTIC_IGNORE 
>> ("-Wself-move")
>>  # define DIAGNOSTIC_IGNORE_DEPRECATED_REGISTER \
>>    DIAGNOSTIC_IGNORE ("-Wdeprecated-register")
>> +# define DIAGNOSTIC_IGNORE_UNUSED_FUNCTION \
>> +  DIAGNOSTIC_IGNORE ("-Wunused-function")
> 
> GCC also understands this warning.  So I think we should define
> the ignore macro for GCC too.
> 
> Now that raises the question of whether you want to suppress the
> warning in vec.h for GCC too.  But that's actually the point
> that made me come here to comment:
> 
> Imagine that we'll want to suppress -Wunused-function with GCC
> in some other source file, for some reason.  At that point we'll
> naturally want to adjust diagnostics.h to define
> 
>  # define DIAGNOSTIC_IGNORE_UNUSED_FUNCTION \
>    DIAGNOSTIC_IGNORE ("-Wunused-function")
> 
> for GCC too instead of leaving DIAGNOSTIC_IGNORE_UNUSED_FUNCTION empty
> for GCC.  But, that will make the existing (at that time) users
> of DIAGNOSTIC_IGNORE_UNUSED_FUNCTION start suppressing the warning
> on GCC too, and we may or not want that.
> 
> This, to me indicates that:
> 
> #1 - The common/diagnostics.h macros define the non-empty "ignore"
>    macro on all compilers that understand it.  Then vec.h does:
> 
> DIAGNOSTIC_PUSH
> #ifdef __clang__
> DIAGNOSTIC_IGNORE_UNUSED_FUNCTION					
> #endif
> 
> #2 - We name the macro something else more targeted for this
> specific use case, and define it to empty for GCC, and to
> -Wunused-function on clang.
> 
> #2.1 - If put on common/diagnostics.h, name it something
>        generic, like:
> 
> /* Suppress -Wunused-function for functions defined in source files as
>    result of expanding macros (that define the functions) defined
>    in headers.  */
> #ifdef __lang
> # define DIAGNOSTIC_IGNORE_UNUSED_FUNCTION_HEADER_MACRO_EXPANSION \
>     DIAGNOSTIC_IGNORE_UNUSED_FUNCTION
> #else
> # define DIAGNOSTIC_IGNORE_UNUSED_FUNCTION_HEADER_MACRO_EXPANSION
> #endif
> 
> #2.2 - Otherwise, define DIAGNOSTIC_IGNORE_UNUSED_FUNCTION
> for for GCC and clang, and put something like this on
> top of vec.h:
> 
> /* Comment describing issue and pointing to clang bug report.  */
> #ifdef __clang__
>  #define DIAGNOSTIC_IGNORE_UNUSED_VEC_FUNCTION \
>     DIAGNOSTIC_IGNORE_UNUSED_FUNCTION
> #else
>  #define DIAGNOSTIC_IGNORE_UNUSED_VEC_FUNCTION
> #endif
> 
> And use that instead of DIAGNOSTIC_IGNORE_FUNCTION.
> 
> I think I prefer #2.2, then #2.1, then #1.
> 
> Thanks,
> Pedro Alves

I wanted to keep it simple and easy to understand, so I didn't want to 
add to many layers of definitions.  I thought that even if we ignored 
-Wunused-function in the vector macro expansions when compiling with 
GCC, it wasn't a big deal.  But 2.2 is fine with me, I'll try it.  It's 
all temporary anyway (famous last words), since the days of vec.h are 
counted :).