diff mbox

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

Message ID 1498412703-24303-3-git-send-email-simon.marchi@ericsson.com
State New, archived
Headers

Commit Message

Simon Marchi June 25, 2017, 5:45 p.m. UTC 
  clang has a too aggressive (or broken, depends on how you want to see
it) -Wunused-function warning, 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(+)

Comments

Pedro Alves June 26, 2017, 9:28 a.m. UTC | #1 
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
Simon Marchi June 26, 2017, 12:28 p.m. UTC | #2 
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 :).
Pedro Alves June 26, 2017, 12:47 p.m. UTC | #3 
On 06/26/2017 01:28 PM, Simon Marchi wrote:

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

That's another option (and I think it should be fine, with a comment),
but it wasn't what the patch was doing, so you'd be leaving deciding
whether that's fine to whoever comes next and wants to make
DIAGNOSTICS_IGNORE_UNUSED_FUNCTION on gcc.  IMO, that's a form of
technical dept that we should avoid.

The point here I'm trying to convey is: avoid letting the specifics
of users of the DIAGNOSTICS_IGNORE users bleed into the conditionals
that are used to define the macros in diagnostics.h.  At least
for the macros that map directly to the warning name.

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

True.  :-)

Thanks,
Pedro Alves
Simon Marchi June 26, 2017, 12:51 p.m. UTC | #4 
On 2017-06-26 14:47, Pedro Alves wrote:
> On 06/26/2017 01:28 PM, Simon Marchi wrote:
> 
>> 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.
> 
> That's another option (and I think it should be fine, with a comment),
> but it wasn't what the patch was doing, so you'd be leaving deciding
> whether that's fine to whoever comes next and wants to make
> DIAGNOSTICS_IGNORE_UNUSED_FUNCTION on gcc.  IMO, that's a form of
> technical dept that we should avoid.

You are right, and it was due to technical laziness :P.
diff mbox

Patch

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")
 #else
 # define DIAGNOSTIC_IGNORE_SELF_MOVE
 # define DIAGNOSTIC_IGNORE_DEPRECATED_REGISTER
+# define DIAGNOSTIC_IGNORE_UNUSED_FUNCTION
 #endif
 
 #endif /* COMMON_DIAGNOSTICS_H */
diff --git a/gdb/common/vec.h b/gdb/common/vec.h
index 982f771..0221c88 100644
--- a/gdb/common/vec.h
+++ b/gdb/common/vec.h
@@ -20,6 +20,8 @@ 
 #if !defined (GDB_VEC_H)
 #define GDB_VEC_H
 
+#include "diagnostics.h"
+
 /* The macros here implement a set of templated vector types and
    associated interfaces.  These templates are implemented with
    macros, as we're not in C++ land.  The interface functions are
@@ -408,6 +410,8 @@  typedef struct VEC(T)							  \
 
 /* Vector of integer-like object.  */
 #define DEF_VEC_I(T)							  \
+DIAGNOSTIC_PUSH 							  \
+DIAGNOSTIC_IGNORE_UNUSED_FUNCTION					  \
 static inline void VEC_OP (T,must_be_integral_type) (void)		  \
 {									  \
   (void)~(T)0;								  \
@@ -416,10 +420,13 @@  static inline void VEC_OP (T,must_be_integral_type) (void)		  \
 VEC_T(T);								  \
 DEF_VEC_FUNC_P(T)							  \
 DEF_VEC_ALLOC_FUNC_I(T)							  \
+DIAGNOSTIC_POP								  \
 struct vec_swallow_trailing_semi
 
 /* Vector of pointer to object.  */
 #define DEF_VEC_P(T)							  \
+DIAGNOSTIC_PUSH 							  \
+DIAGNOSTIC_IGNORE_UNUSED_FUNCTION					  \
 static inline void VEC_OP (T,must_be_pointer_type) (void)		  \
 {									  \
   (void)((T)1 == (void *)1);						  \
@@ -428,13 +435,17 @@  static inline void VEC_OP (T,must_be_pointer_type) (void)		  \
 VEC_T(T);								  \
 DEF_VEC_FUNC_P(T)							  \
 DEF_VEC_ALLOC_FUNC_P(T)							  \
+DIAGNOSTIC_POP								  \
 struct vec_swallow_trailing_semi
 
 /* Vector of object.  */
 #define DEF_VEC_O(T)							  \
+DIAGNOSTIC_PUSH 							  \
+DIAGNOSTIC_IGNORE_UNUSED_FUNCTION					  \
 VEC_T(T);								  \
 DEF_VEC_FUNC_O(T)							  \
 DEF_VEC_ALLOC_FUNC_O(T)							  \
+DIAGNOSTIC_POP								  \
 struct vec_swallow_trailing_semi
 
 /* Avoid offsetof (or its usual C implementation) as it triggers