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

Simon Marchi
2017-06-26T12:28:24Z

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 :).