* [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables @ 2015-09-11 22:29 Mark Wielaard 2015-09-11 22:40 ` Bernd Schmidt 0 siblings, 1 reply; 43+ messages in thread From: Mark Wielaard @ 2015-09-11 22:29 UTC (permalink / raw) To: gcc-patches; +Cc: Mark Wielaard 12 years ago it was decided that -Wunused-variable shouldn't warn about static const variables because some code used const static char rcsid[] strings which were never used but wanted in the code anyway. But as the bug points out this hides some real bugs. These days the usage of rcsids is not very popular anymore. So this patch changes the default to warn about unused static const variables with -Wunused-variable. And it adds a new option -Wno-unused-const-variable to turn this warning off. New testcases are included to test the new warning with -Wunused-variable and suppressing it with -Wno-unused-const-variable or unused attribute. gcc/ChangeLog PR c/28901 * common.opt (Wunused-const-variable): New option. * toplev.c (check_global_declaration): Check and use warn_unused_const_variable. * doc/invoke.texi (Warning Options): Add -Wunused-const-variable. (-Wunused-variable): Remove non-constant. Implies -Wunused-const-variable. (-Wunused-const-variable): New. gcc/testsuite/ChangeLog PR c/28901 * gcc.dg/unused-4.c: Adjust warning for static const. * gcc.dg/unused-variable-1.c: New test. * gcc.dg/unused-variable-2.c: Likewise. Tested on x86_64-pc-linux-gnu, no regressions. --- diff --git a/gcc/common.opt b/gcc/common.opt index 94d1d88..ae2fe77 100644 --- a/gcc/common.opt +++ b/gcc/common.opt @@ -735,6 +735,10 @@ Wunused-variable Common Var(warn_unused_variable) Warning EnabledBy(Wunused) Warn when a variable is unused +Wunused-const-variable +Common Var(warn_unused_const_variable) Warning EnabledBy(Wunused-variable) +Warn when a const variable is unused + Wcoverage-mismatch Common Var(warn_coverage_mismatch) Init(1) Warning Warn in case profiles in -fprofile-use do not match diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index 518d689..211b9e1 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -290,6 +290,7 @@ Objective-C and Objective-C++ Dialects}. -Wunsuffixed-float-constants -Wunused -Wunused-function @gol -Wunused-label -Wunused-local-typedefs -Wunused-parameter @gol -Wno-unused-result -Wunused-value @gol -Wunused-variable @gol +-Wunused-const-variable @gol -Wunused-but-set-parameter -Wunused-but-set-variable @gol -Wuseless-cast -Wvariadic-macros -Wvector-operation-performance @gol -Wvla -Wvolatile-register-var -Wwrite-strings @gol @@ -4143,13 +4144,22 @@ its return value. The default is @option{-Wunused-result}. @item -Wunused-variable @opindex Wunused-variable @opindex Wno-unused-variable -Warn whenever a local variable or non-constant static variable is unused -aside from its declaration. +Warn whenever a local or static variable is unused aside from its +declaration. This option implies @option{-Wunused-const-variable}. This warning is enabled by @option{-Wall}. To suppress this warning use the @code{unused} attribute (@pxref{Variable Attributes}). +@item -Wunused-const-variable +@opindex Wunused-const-variable +@opindex Wno-unused-const-variable +Warn whenever a constant static variable is unused aside from its declaration. +This warning is enabled by @option{-Wunused-variable}. + +To suppress this warning use the @code{unused} attribute +(@pxref{Variable Attributes}). + @item -Wunused-value @opindex Wunused-value @opindex Wno-unused-value diff --git a/gcc/testsuite/gcc.dg/unused-4.c b/gcc/testsuite/gcc.dg/unused-4.c index 99e845f..5323600 100644 --- a/gcc/testsuite/gcc.dg/unused-4.c +++ b/gcc/testsuite/gcc.dg/unused-4.c @@ -1,6 +1,6 @@ /* { dg-do compile } */ /* { dg-options "-Wunused -O3" } */ -static const int i = 0; +static const int i = 0; /* { dg-warning "defined but not used" } */ static void f() { } /* { dg-warning "defined but not used" } */ static inline void g() { } diff --git a/gcc/testsuite/gcc.dg/unused-variable-1.c b/gcc/testsuite/gcc.dg/unused-variable-1.c new file mode 100644 index 0000000..cb86c3bc --- /dev/null +++ b/gcc/testsuite/gcc.dg/unused-variable-1.c @@ -0,0 +1,7 @@ +/* { dg-do compile } */ +/* { dg-options "-Wunused-variable" } */ + +static int a = 0; /* { dg-warning "defined but not used" } */ +static const int b = 0; /* { dg-warning "defined but not used" } */ +static int c __attribute__ ((unused)) = 0; +static const char rcsid[] __attribute__ ((unused)) = "version-string"; diff --git a/gcc/testsuite/gcc.dg/unused-variable-2.c b/gcc/testsuite/gcc.dg/unused-variable-2.c new file mode 100644 index 0000000..0496466 --- /dev/null +++ b/gcc/testsuite/gcc.dg/unused-variable-2.c @@ -0,0 +1,7 @@ +/* { dg-do compile } */ +/* { dg-options "-Wunused-variable -Wno-unused-const-variable" } */ + +static int a = 0; /* { dg-warning "defined but not used" } */ +static const int b = 0; +static int c __attribute__ ((unused)) = 0; +static const char rcsid[] = "version-string"; diff --git a/gcc/toplev.c b/gcc/toplev.c index 926224a..95e4c52 100644 --- a/gcc/toplev.c +++ b/gcc/toplev.c @@ -497,10 +497,9 @@ check_global_declaration (tree decl) /* Warn about static fns or vars defined but not used. */ if (((warn_unused_function && TREE_CODE (decl) == FUNCTION_DECL) - /* We don't warn about "static const" variables because the - "rcs_id" idiom uses that construction. */ - || (warn_unused_variable - && TREE_CODE (decl) == VAR_DECL && ! TREE_READONLY (decl))) + || (((warn_unused_variable && ! TREE_READONLY (decl)) + || (warn_unused_const_variable && TREE_READONLY (decl))) + && TREE_CODE (decl) == VAR_DECL)) && ! DECL_IN_SYSTEM_HEADER (decl) && ! snode->referred_to_p (/*include_self=*/false) /* This TREE_USED check is needed in addition to referred_to_p @@ -527,7 +526,9 @@ check_global_declaration (tree decl) warning_at (DECL_SOURCE_LOCATION (decl), (TREE_CODE (decl) == FUNCTION_DECL) ? OPT_Wunused_function - : OPT_Wunused_variable, + : (TREE_READONLY (decl) + ? OPT_Wunused_const_variable + : OPT_Wunused_variable), "%qD defined but not used", decl); } ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables 2015-09-11 22:29 [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables Mark Wielaard @ 2015-09-11 22:40 ` Bernd Schmidt 2015-09-13 5:44 ` Mark Wielaard 0 siblings, 1 reply; 43+ messages in thread From: Bernd Schmidt @ 2015-09-11 22:40 UTC (permalink / raw) To: Mark Wielaard, gcc-patches On 09/12/2015 12:12 AM, Mark Wielaard wrote: > 12 years ago it was decided that -Wunused-variable shouldn't warn about > static const variables because some code used const static char rcsid[] > strings which were never used but wanted in the code anyway. But as the > bug points out this hides some real bugs. These days the usage of rcsids > is not very popular anymore. So this patch changes the default to warn > about unused static const variables with -Wunused-variable. And it adds > a new option -Wno-unused-const-variable to turn this warning off. New > testcases are included to test the new warning with -Wunused-variable > and suppressing it with -Wno-unused-const-variable or unused attribute. > PR c/28901 > * gcc.dg/unused-4.c: Adjust warning for static const. > * gcc.dg/unused-variable-1.c: New test. > * gcc.dg/unused-variable-2.c: Likewise. Should these go into c-c++-common? Otherwise I'm ok with the patch, please wait a few days to see if there are objections to this change then commit. Bernd ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables 2015-09-11 22:40 ` Bernd Schmidt @ 2015-09-13 5:44 ` Mark Wielaard 2015-09-13 12:51 ` Mark Wielaard 0 siblings, 1 reply; 43+ messages in thread From: Mark Wielaard @ 2015-09-13 5:44 UTC (permalink / raw) To: Bernd Schmidt; +Cc: gcc-patches On Sat, Sep 12, 2015 at 12:29:05AM +0200, Bernd Schmidt wrote: > On 09/12/2015 12:12 AM, Mark Wielaard wrote: > >12 years ago it was decided that -Wunused-variable shouldn't warn about > >static const variables because some code used const static char rcsid[] > >strings which were never used but wanted in the code anyway. But as the > >bug points out this hides some real bugs. These days the usage of rcsids > >is not very popular anymore. So this patch changes the default to warn > >about unused static const variables with -Wunused-variable. And it adds > >a new option -Wno-unused-const-variable to turn this warning off. New > >testcases are included to test the new warning with -Wunused-variable > >and suppressing it with -Wno-unused-const-variable or unused attribute. > > > PR c/28901 > > * gcc.dg/unused-4.c: Adjust warning for static const. > > * gcc.dg/unused-variable-1.c: New test. > > * gcc.dg/unused-variable-2.c: Likewise. > > Should these go into c-c++-common? No. It is C only. But I realize that isn't really clear from my patch nor from the documentation I wrote for it. Since in C++ a const isn't by default file scoped and const variables always need to be initialized they are used differently than in C. Where in C you would use a #define in C++ you would use a const. So the cxx_warn_unused_global_decl () lang hook explicitly says to not warn about them. Although I think that is correct, I now think it is confusing you cannot enable the warning for C++ if you really want to. It should be off by default for C++, but on by default for C when -Wunused-variable is enabled. But it would actually be nice to be able to use it too for C++ if the user really wants to instead of having the warning suppression for C++ hardcoded. > Otherwise I'm ok with the patch, please > wait a few days to see if there are objections to this change then commit. I'll rewrite my patch a little, add some C++ testcases, and update the documentation. Then we can discuss again. Thanks, Mark ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables 2015-09-13 5:44 ` Mark Wielaard @ 2015-09-13 12:51 ` Mark Wielaard 2015-09-13 13:36 ` Manuel López-Ibáñez 2015-09-17 12:41 ` Gerald Pfeifer 0 siblings, 2 replies; 43+ messages in thread From: Mark Wielaard @ 2015-09-13 12:51 UTC (permalink / raw) To: Bernd Schmidt; +Cc: gcc-patches [-- Attachment #1: Type: text/plain, Size: 506 bytes --] On Sun, Sep 13, 2015 at 12:00:51AM +0200, Mark Wielaard wrote: > I'll rewrite my patch a little, add some C++ testcases, and update the > documentation. Then we can discuss again. Slightly adjusted patch attached. Now it is explicit that the warning is enabled by -Wunused-variable for C, but not C++. There are testcases for both C and C++ to check the defaults. And the hardcoded override is removed for C++, so the user could enable it if they want. Regtested on x86_64-pc-linux-gnu only new PASSes. [-- Attachment #2: PR28901-Wunused-variable-ignores-unused-const-initia.patch --] [-- Type: text/plain, Size: 9181 bytes --] From 609e4495b6d63a0212e9964064f47f7b26c69b7e Mon Sep 17 00:00:00 2001 From: Mark Wielaard <mjw@redhat.com> Date: Fri, 11 Sep 2015 23:54:15 +0200 Subject: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables in C 12 years ago it was decided that -Wunused-variable shouldn't warn about static const variables because some code used const static char rcsid[] strings which were never used but wanted in the code anyway. But as the bug points out this hides some real bugs. These days the usage of rcsids is not very popular anymore. So this patch changes the default to warn about unused static const variables in C with -Wunused-variable. And it adds a new option -Wno-unused-const-variable to turn this warning off. For C++ this new warning is off by default, since const variables can be used as #defines in C++. New testcases for the new defaults in C and C++ are included testing the new warning and suppressing it with an unused attribute or using -Wno-unused-const-variable. gcc/ChangeLog PR c/28901 * common.opt (Wunused-const-variable): New option. * toplev.c (check_global_declaration): Check and use warn_unused_const_variable_set. * doc/invoke.texi (Warning Options): Add -Wunused-const-variable. (-Wunused-variable): Remove non-constant. For C mplies -Wunused-const-variable. (-Wunused-const-variable): New. gcc/c-family/ChangeLog PR c/28901 * c-opts.c (c_common_post_options): warn_unused_const_variable_set is set when not explicitly set for C when warn_unused_variable is set. gcc/cp/ChangeLog PR c/28901 * cp-objcp-common.c (cxx_warn_unused_global_decl): Remove hard-coded VAR_P TREE_READONLY override. gcc/testsuite/ChangeLog PR c/28901 * g++.dg/warn/unused-variable-1.C: New test. * g++.dg/warn/unused-variable-2.C: Likewise. * gcc.dg/unused-4.c: Adjust warning for static const. * gcc.dg/unused-variable-1.c: New test. * gcc.dg/unused-variable-2.c: Likewise. --- diff --git a/gcc/c-family/c-opts.c b/gcc/c-family/c-opts.c index 3239a85..f8a5aa1 100644 --- a/gcc/c-family/c-opts.c +++ b/gcc/c-family/c-opts.c @@ -862,6 +862,12 @@ c_common_post_options (const char **pfilename) warn_shift_negative_value = (extra_warnings && (cxx_dialect >= cxx11 || flag_isoc99)); + /* -Wunused-variable implies -Wunused-const-variable for C, but not + for C++, because const variables take the place of #defines in C++. */ + if (warn_unused_const_variable_set == -1) + warn_unused_const_variable_set = (warn_unused_variable + && ! c_dialect_cxx ()); + /* Declone C++ 'structors if -Os. */ if (flag_declone_ctor_dtor == -1) flag_declone_ctor_dtor = optimize_size; diff --git a/gcc/common.opt b/gcc/common.opt index 94d1d88..c85f1b9 100644 --- a/gcc/common.opt +++ b/gcc/common.opt @@ -735,6 +735,10 @@ Wunused-variable Common Var(warn_unused_variable) Warning EnabledBy(Wunused) Warn when a variable is unused +Wunused-const-variable +Common Var(warn_unused_const_variable_set) Init(-1) Warning +Warn when a const variable is unused + Wcoverage-mismatch Common Var(warn_coverage_mismatch) Init(1) Warning Warn in case profiles in -fprofile-use do not match diff --git a/gcc/cp/cp-objcp-common.c b/gcc/cp/cp-objcp-common.c index 2cab89c..808defd 100644 --- a/gcc/cp/cp-objcp-common.c +++ b/gcc/cp/cp-objcp-common.c @@ -62,10 +62,6 @@ cxx_warn_unused_global_decl (const_tree decl) if (DECL_IN_SYSTEM_HEADER (decl)) return false; - /* Const variables take the place of #defines in C++. */ - if (VAR_P (decl) && TREE_READONLY (decl)) - return false; - return true; } diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index 518d689..7b5e44e 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -290,6 +290,7 @@ Objective-C and Objective-C++ Dialects}. -Wunsuffixed-float-constants -Wunused -Wunused-function @gol -Wunused-label -Wunused-local-typedefs -Wunused-parameter @gol -Wno-unused-result -Wunused-value @gol -Wunused-variable @gol +-Wunused-const-variable @gol -Wunused-but-set-parameter -Wunused-but-set-variable @gol -Wuseless-cast -Wvariadic-macros -Wvector-operation-performance @gol -Wvla -Wvolatile-register-var -Wwrite-strings @gol @@ -4143,9 +4144,20 @@ its return value. The default is @option{-Wunused-result}. @item -Wunused-variable @opindex Wunused-variable @opindex Wno-unused-variable -Warn whenever a local variable or non-constant static variable is unused -aside from its declaration. -This warning is enabled by @option{-Wall}. +Warn whenever a local or static variable is unused aside from its +declaration. This option implies @option{-Wunused-const-variable} for C, +but not for C++. This warning is enabled by @option{-Wall}. + +To suppress this warning use the @code{unused} attribute +(@pxref{Variable Attributes}). + +@item -Wunused-const-variable +@opindex Wunused-const-variable +@opindex Wno-unused-const-variable +Warn whenever a constant static variable is unused aside from its declaration. +This warning is enabled by @option{-Wunused-variable} for C, but not for C++. +In C++ this is normally not an error since const variables take the place of +@code{#define}s in C++. To suppress this warning use the @code{unused} attribute (@pxref{Variable Attributes}). diff --git a/gcc/testsuite/g++.dg/warn/unused-variable-1.C b/gcc/testsuite/g++.dg/warn/unused-variable-1.C new file mode 100644 index 0000000..cf531c0 --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/unused-variable-1.C @@ -0,0 +1,7 @@ +/* { dg-do compile } */ +/* { dg-options "-Wunused-variable" } */ + +static int a = 0; /* { dg-warning "defined but not used" } */ +static const int b = 0; /* Unlike C, this doesn't cause a warning in C++. */ +static int c __attribute__ ((unused)) = 0; +static const char rcsid[] = "version-string"; /* Likewise. */ diff --git a/gcc/testsuite/g++.dg/warn/unused-variable-2.C b/gcc/testsuite/g++.dg/warn/unused-variable-2.C new file mode 100644 index 0000000..b608fbc --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/unused-variable-2.C @@ -0,0 +1,7 @@ +/* { dg-do compile } */ +/* { dg-options "-Wunused-variable -Wunused-const-variable" } */ + +static int a = 0; /* { dg-warning "defined but not used" } */ +static const int b = 0; /* { dg-warning "defined but not used" } */ +static int c __attribute__ ((unused)) = 0; +static const char rcsid[] __attribute__ ((unused)) = "version-string"; diff --git a/gcc/testsuite/gcc.dg/unused-4.c b/gcc/testsuite/gcc.dg/unused-4.c index 99e845f..5323600 100644 --- a/gcc/testsuite/gcc.dg/unused-4.c +++ b/gcc/testsuite/gcc.dg/unused-4.c @@ -1,6 +1,6 @@ /* { dg-do compile } */ /* { dg-options "-Wunused -O3" } */ -static const int i = 0; +static const int i = 0; /* { dg-warning "defined but not used" } */ static void f() { } /* { dg-warning "defined but not used" } */ static inline void g() { } diff --git a/gcc/testsuite/gcc.dg/unused-variable-1.c b/gcc/testsuite/gcc.dg/unused-variable-1.c new file mode 100644 index 0000000..cb86c3bc --- /dev/null +++ b/gcc/testsuite/gcc.dg/unused-variable-1.c @@ -0,0 +1,7 @@ +/* { dg-do compile } */ +/* { dg-options "-Wunused-variable" } */ + +static int a = 0; /* { dg-warning "defined but not used" } */ +static const int b = 0; /* { dg-warning "defined but not used" } */ +static int c __attribute__ ((unused)) = 0; +static const char rcsid[] __attribute__ ((unused)) = "version-string"; diff --git a/gcc/testsuite/gcc.dg/unused-variable-2.c b/gcc/testsuite/gcc.dg/unused-variable-2.c new file mode 100644 index 0000000..0496466 --- /dev/null +++ b/gcc/testsuite/gcc.dg/unused-variable-2.c @@ -0,0 +1,7 @@ +/* { dg-do compile } */ +/* { dg-options "-Wunused-variable -Wno-unused-const-variable" } */ + +static int a = 0; /* { dg-warning "defined but not used" } */ +static const int b = 0; +static int c __attribute__ ((unused)) = 0; +static const char rcsid[] = "version-string"; diff --git a/gcc/toplev.c b/gcc/toplev.c index 926224a..83cafed 100644 --- a/gcc/toplev.c +++ b/gcc/toplev.c @@ -497,10 +497,9 @@ check_global_declaration (tree decl) /* Warn about static fns or vars defined but not used. */ if (((warn_unused_function && TREE_CODE (decl) == FUNCTION_DECL) - /* We don't warn about "static const" variables because the - "rcs_id" idiom uses that construction. */ - || (warn_unused_variable - && TREE_CODE (decl) == VAR_DECL && ! TREE_READONLY (decl))) + || (((warn_unused_variable && ! TREE_READONLY (decl)) + || (warn_unused_const_variable_set > 0 && TREE_READONLY (decl))) + && TREE_CODE (decl) == VAR_DECL)) && ! DECL_IN_SYSTEM_HEADER (decl) && ! snode->referred_to_p (/*include_self=*/false) /* This TREE_USED check is needed in addition to referred_to_p @@ -527,7 +526,9 @@ check_global_declaration (tree decl) warning_at (DECL_SOURCE_LOCATION (decl), (TREE_CODE (decl) == FUNCTION_DECL) ? OPT_Wunused_function - : OPT_Wunused_variable, + : (TREE_READONLY (decl) + ? OPT_Wunused_const_variable + : OPT_Wunused_variable), "%qD defined but not used", decl); } ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables 2015-09-13 12:51 ` Mark Wielaard @ 2015-09-13 13:36 ` Manuel López-Ibáñez 2015-09-13 18:50 ` Mark Wielaard 2015-09-17 12:41 ` Gerald Pfeifer 1 sibling, 1 reply; 43+ messages in thread From: Manuel López-Ibáñez @ 2015-09-13 13:36 UTC (permalink / raw) To: Mark Wielaard; +Cc: Bernd Schmidt, GCC Patches On 13/09/15 13:40, Mark Wielaard wrote: > On Sun, Sep 13, 2015 at 12:00:51AM +0200, Mark Wielaard wrote: >> I'll rewrite my patch a little, add some C++ testcases, and update the >> documentation. Then we can discuss again. > > Slightly adjusted patch attached. Now it is explicit that the warning is > enabled by -Wunused-variable for C, but not C++. There are testcases for > both C and C++ to check the defaults. And the hardcoded override is > removed for C++, so the user could enable it if they want. > + /* -Wunused-variable implies -Wunused-const-variable for C, but not > + for C++, because const variables take the place of #defines in C++. */ > + if (warn_unused_const_variable_set == -1) > + warn_unused_const_variable_set = (warn_unused_variable > + && ! c_dialect_cxx ()); > + Can't you use LangEnabledBy() in c.opt to implement this? If not, I think this would be a regression or a bug. Ideally, all options dependences should be encoded in the .opt files, however, it cannot handle yet conditions such as (cxx_dialect >= cxx11 || flag_isoc99). But it can handle a lot of other things already. See https://gcc.gnu.org/onlinedocs/gccint/Option-properties.html Cheers, Manuel. ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables 2015-09-13 13:36 ` Manuel López-Ibáñez @ 2015-09-13 18:50 ` Mark Wielaard 2015-09-14 7:54 ` Bernd Schmidt 0 siblings, 1 reply; 43+ messages in thread From: Mark Wielaard @ 2015-09-13 18:50 UTC (permalink / raw) To: Manuel López-Ibáñez; +Cc: Bernd Schmidt, GCC Patches [-- Attachment #1: Type: text/plain, Size: 898 bytes --] On Sun, Sep 13, 2015 at 02:50:53PM +0200, Manuel López-Ibáñez wrote: > On 13/09/15 13:40, Mark Wielaard wrote: > >Slightly adjusted patch attached. Now it is explicit that the warning is > >enabled by -Wunused-variable for C, but not C++. There are testcases for > >both C and C++ to check the defaults. And the hardcoded override is > >removed for C++, so the user could enable it if they want. > > >+ /* -Wunused-variable implies -Wunused-const-variable for C, but not > >+ for C++, because const variables take the place of #defines in C++. */ > >+ if (warn_unused_const_variable_set == -1) > >+ warn_unused_const_variable_set = (warn_unused_variable > >+ && ! c_dialect_cxx ()); > >+ > > Can't you use LangEnabledBy() in c.opt to implement this? Yes, I can. See simplified patch attached. I wasn't aware there was already such a nice framework for this. Thanks, Mark [-- Attachment #2: newer-Wunused-variable-ignores-unused-const-initia.patch --] [-- Type: text/plain, Size: 8806 bytes --] commit 97505bd0e4ac15d86c2a302cfebc5f1a4fc2c2e8 Author: Mark Wielaard <mjw@redhat.com> Date: Fri Sep 11 23:54:15 2015 +0200 PR28901 -Wunused-variable ignores unused const initialised variables in C 12 years ago it was decided that -Wunused-variable shouldn't warn about static const variables because some code used const static char rcsid[] strings which were never used but wanted in the code anyway. But as the bug points out this hides some real bugs. These days the usage of rcsids is not very popular anymore. So this patch changes the default to warn about unused static const variables in C with -Wunused-variable. And it adds a new option -Wno-unused-const-variable to turn this warning off. For C++ this new warning is off by default, since const variables can be used as #defines in C++. New testcases for the new defaults in C and C++ are included testing the new warning and suppressing it with an unused attribute or using -Wno-unused-const-variable. gcc/ChangeLog PR c/28901 * toplev.c (check_global_declaration): Check and use warn_unused_const_variable. * doc/invoke.texi (Warning Options): Add -Wunused-const-variable. (-Wunused-variable): Remove non-constant. For C implies -Wunused-const-variable. (-Wunused-const-variable): New. gcc/c-family/ChangeLog PR c/28901 * c.opt (Wunused-variable): Option from common.opt. (Wunused-const-variable): New option. gcc/cp/ChangeLog PR c/28901 * cp-objcp-common.c (cxx_warn_unused_global_decl): Remove hard-coded VAR_P TREE_READONLY override. gcc/testsuite/ChangeLog PR c/28901 * g++.dg/warn/unused-variable-1.C: New test. * g++.dg/warn/unused-variable-2.C: Likewise. * gcc.dg/unused-4.c: Adjust warning for static const. * gcc.dg/unused-variable-1.c: New test. * gcc.dg/unused-variable-2.c: Likewise. diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt index d519d7a..47ba070 100644 --- a/gcc/c-family/c.opt +++ b/gcc/c-family/c.opt @@ -912,6 +912,14 @@ Wunused-result C ObjC C++ ObjC++ Var(warn_unused_result) Init(1) Warning Warn if a caller of a function, marked with attribute warn_unused_result, does not use its return value +Wunused-variable +C ObjC C++ ObjC++ LangEnabledBy(C ObjC C++ ObjC++,Wunused) +; documented in common.opt + +Wunused-const-variable +C ObjC C++ ObjC++ Var(warn_unused_const_variable) Warning LangEnabledBy(C ObjC,Wunused-variable) +Warn when a const variable is unused + Wvariadic-macros C ObjC C++ ObjC++ CPP(warn_variadic_macros) CppReason(CPP_W_VARIADIC_MACROS) Var(cpp_warn_variadic_macros) Init(0) Warning LangEnabledBy(C ObjC C++ ObjC++,Wpedantic || Wtraditional) Warn about using variadic macros diff --git a/gcc/cp/cp-objcp-common.c b/gcc/cp/cp-objcp-common.c index 2cab89c..808defd 100644 --- a/gcc/cp/cp-objcp-common.c +++ b/gcc/cp/cp-objcp-common.c @@ -62,10 +62,6 @@ cxx_warn_unused_global_decl (const_tree decl) if (DECL_IN_SYSTEM_HEADER (decl)) return false; - /* Const variables take the place of #defines in C++. */ - if (VAR_P (decl) && TREE_READONLY (decl)) - return false; - return true; } diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index 518d689..7b5e44e 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -290,6 +290,7 @@ Objective-C and Objective-C++ Dialects}. -Wunsuffixed-float-constants -Wunused -Wunused-function @gol -Wunused-label -Wunused-local-typedefs -Wunused-parameter @gol -Wno-unused-result -Wunused-value @gol -Wunused-variable @gol +-Wunused-const-variable @gol -Wunused-but-set-parameter -Wunused-but-set-variable @gol -Wuseless-cast -Wvariadic-macros -Wvector-operation-performance @gol -Wvla -Wvolatile-register-var -Wwrite-strings @gol @@ -4143,9 +4144,20 @@ its return value. The default is @option{-Wunused-result}. @item -Wunused-variable @opindex Wunused-variable @opindex Wno-unused-variable -Warn whenever a local variable or non-constant static variable is unused -aside from its declaration. -This warning is enabled by @option{-Wall}. +Warn whenever a local or static variable is unused aside from its +declaration. This option implies @option{-Wunused-const-variable} for C, +but not for C++. This warning is enabled by @option{-Wall}. + +To suppress this warning use the @code{unused} attribute +(@pxref{Variable Attributes}). + +@item -Wunused-const-variable +@opindex Wunused-const-variable +@opindex Wno-unused-const-variable +Warn whenever a constant static variable is unused aside from its declaration. +This warning is enabled by @option{-Wunused-variable} for C, but not for C++. +In C++ this is normally not an error since const variables take the place of +@code{#define}s in C++. To suppress this warning use the @code{unused} attribute (@pxref{Variable Attributes}). diff --git a/gcc/testsuite/g++.dg/warn/unused-variable-1.C b/gcc/testsuite/g++.dg/warn/unused-variable-1.C new file mode 100644 index 0000000..cf531c0 --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/unused-variable-1.C @@ -0,0 +1,7 @@ +/* { dg-do compile } */ +/* { dg-options "-Wunused-variable" } */ + +static int a = 0; /* { dg-warning "defined but not used" } */ +static const int b = 0; /* Unlike C, this doesn't cause a warning in C++. */ +static int c __attribute__ ((unused)) = 0; +static const char rcsid[] = "version-string"; /* Likewise. */ diff --git a/gcc/testsuite/g++.dg/warn/unused-variable-2.C b/gcc/testsuite/g++.dg/warn/unused-variable-2.C new file mode 100644 index 0000000..b608fbc --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/unused-variable-2.C @@ -0,0 +1,7 @@ +/* { dg-do compile } */ +/* { dg-options "-Wunused-variable -Wunused-const-variable" } */ + +static int a = 0; /* { dg-warning "defined but not used" } */ +static const int b = 0; /* { dg-warning "defined but not used" } */ +static int c __attribute__ ((unused)) = 0; +static const char rcsid[] __attribute__ ((unused)) = "version-string"; diff --git a/gcc/testsuite/gcc.dg/unused-4.c b/gcc/testsuite/gcc.dg/unused-4.c index 99e845f..5323600 100644 --- a/gcc/testsuite/gcc.dg/unused-4.c +++ b/gcc/testsuite/gcc.dg/unused-4.c @@ -1,6 +1,6 @@ /* { dg-do compile } */ /* { dg-options "-Wunused -O3" } */ -static const int i = 0; +static const int i = 0; /* { dg-warning "defined but not used" } */ static void f() { } /* { dg-warning "defined but not used" } */ static inline void g() { } diff --git a/gcc/testsuite/gcc.dg/unused-variable-1.c b/gcc/testsuite/gcc.dg/unused-variable-1.c new file mode 100644 index 0000000..cb86c3bc --- /dev/null +++ b/gcc/testsuite/gcc.dg/unused-variable-1.c @@ -0,0 +1,7 @@ +/* { dg-do compile } */ +/* { dg-options "-Wunused-variable" } */ + +static int a = 0; /* { dg-warning "defined but not used" } */ +static const int b = 0; /* { dg-warning "defined but not used" } */ +static int c __attribute__ ((unused)) = 0; +static const char rcsid[] __attribute__ ((unused)) = "version-string"; diff --git a/gcc/testsuite/gcc.dg/unused-variable-2.c b/gcc/testsuite/gcc.dg/unused-variable-2.c new file mode 100644 index 0000000..0496466 --- /dev/null +++ b/gcc/testsuite/gcc.dg/unused-variable-2.c @@ -0,0 +1,7 @@ +/* { dg-do compile } */ +/* { dg-options "-Wunused-variable -Wno-unused-const-variable" } */ + +static int a = 0; /* { dg-warning "defined but not used" } */ +static const int b = 0; +static int c __attribute__ ((unused)) = 0; +static const char rcsid[] = "version-string"; diff --git a/gcc/toplev.c b/gcc/toplev.c index 926224a..95e4c52 100644 --- a/gcc/toplev.c +++ b/gcc/toplev.c @@ -497,10 +497,9 @@ check_global_declaration (tree decl) /* Warn about static fns or vars defined but not used. */ if (((warn_unused_function && TREE_CODE (decl) == FUNCTION_DECL) - /* We don't warn about "static const" variables because the - "rcs_id" idiom uses that construction. */ - || (warn_unused_variable - && TREE_CODE (decl) == VAR_DECL && ! TREE_READONLY (decl))) + || (((warn_unused_variable && ! TREE_READONLY (decl)) + || (warn_unused_const_variable && TREE_READONLY (decl))) + && TREE_CODE (decl) == VAR_DECL)) && ! DECL_IN_SYSTEM_HEADER (decl) && ! snode->referred_to_p (/*include_self=*/false) /* This TREE_USED check is needed in addition to referred_to_p @@ -527,7 +526,9 @@ check_global_declaration (tree decl) warning_at (DECL_SOURCE_LOCATION (decl), (TREE_CODE (decl) == FUNCTION_DECL) ? OPT_Wunused_function - : OPT_Wunused_variable, + : (TREE_READONLY (decl) + ? OPT_Wunused_const_variable + : OPT_Wunused_variable), "%qD defined but not used", decl); } ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables 2015-09-13 18:50 ` Mark Wielaard @ 2015-09-14 7:54 ` Bernd Schmidt 2015-09-15 17:04 ` Steve Ellcey 0 siblings, 1 reply; 43+ messages in thread From: Bernd Schmidt @ 2015-09-14 7:54 UTC (permalink / raw) To: Mark Wielaard, Manuel López-Ibáñez; +Cc: GCC Patches On 09/13/2015 08:24 PM, Mark Wielaard wrote: > commit 97505bd0e4ac15d86c2a302cfebc5f1a4fc2c2e8 > Author: Mark Wielaard<mjw@redhat.com> > Date: Fri Sep 11 23:54:15 2015 +0200 > > PR28901 -Wunused-variable ignores unused const initialised variables in C This is ok. Bernd ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables 2015-09-14 7:54 ` Bernd Schmidt @ 2015-09-15 17:04 ` Steve Ellcey 2015-09-15 17:10 ` Jakub Jelinek 2015-09-15 17:20 ` [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables Joseph Myers 0 siblings, 2 replies; 43+ messages in thread From: Steve Ellcey @ 2015-09-15 17:04 UTC (permalink / raw) To: Bernd Schmidt Cc: Mark Wielaard, Manuel López-Ibáñez, GCC Patches On Mon, 2015-09-14 at 09:50 +0200, Bernd Schmidt wrote: > On 09/13/2015 08:24 PM, Mark Wielaard wrote: > > commit 97505bd0e4ac15d86c2a302cfebc5f1a4fc2c2e8 > > Author: Mark Wielaard<mjw@redhat.com> > > Date: Fri Sep 11 23:54:15 2015 +0200 > > > > PR28901 -Wunused-variable ignores unused const initialised variables in C > > This is ok. > > > Bernd I am not sure I like this change. It broke the GLIBC build for me on MIPS. Basically GLIBC has a header file with initialized static constant globals (sysdeps/ieee754/dbl-64/atnat2.h contains tqpi1 and qpi1) and that header file is included in multiple .c files like sysdeps/ieee754/dbl-64/e_atan2.c that use some, but not all, of those static constant variables. But between the various .c files all of the globals are used somewhere, just not in every individual .c file. This seems like a perfectly reasonable use of static globals and header files that should not be identified as a warning. This warning causes the GLIBC build to fail because GLIBC is compiled with -Wall -Werror. Steve Ellcey sellcey@imgtec.com % cat a.h static const int a = 3; static const int b = 5; static const int c = 4; % cat a.c #include "a.h" int foo() { return a + b;} % gcc -O2 -Werror -Wall -c a.c In file included from a.c:1:0: a.h:3:18: error: 'c' defined but not used [-Werror=unused-const-variable] static const int c = 4; ^ cc1: all warnings being treated as errors ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables 2015-09-15 17:04 ` Steve Ellcey @ 2015-09-15 17:10 ` Jakub Jelinek 2015-09-15 17:26 ` Steve Ellcey 2015-09-15 17:20 ` [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables Joseph Myers 1 sibling, 1 reply; 43+ messages in thread From: Jakub Jelinek @ 2015-09-15 17:10 UTC (permalink / raw) To: Steve Ellcey Cc: Bernd Schmidt, Mark Wielaard, Manuel López-Ibáñez, GCC Patches On Tue, Sep 15, 2015 at 10:02:15AM -0700, Steve Ellcey wrote: > I am not sure I like this change. It broke the GLIBC build for me on > MIPS. Basically GLIBC has a header file with initialized static > constant globals (sysdeps/ieee754/dbl-64/atnat2.h contains tqpi1 and > qpi1) and that header file is included in multiple .c files like Multiple? All I can see is e_atan2.c including that header file, nothing else. > sysdeps/ieee754/dbl-64/e_atan2.c that use some, but not all, of those > static constant variables. But between the various .c files all of the > globals are used somewhere, just not in every individual .c file. This > seems like a perfectly reasonable use of static globals and header files > that should not be identified as a warning. I disagree. While const vars are special in C++, it is really like any other variable in C, so the warning is IMHO appropriate. Jakub ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables 2015-09-15 17:10 ` Jakub Jelinek @ 2015-09-15 17:26 ` Steve Ellcey 2015-09-15 18:55 ` Mark Wielaard 2015-09-19 2:57 ` Martin Sebor 0 siblings, 2 replies; 43+ messages in thread From: Steve Ellcey @ 2015-09-15 17:26 UTC (permalink / raw) To: Jakub Jelinek Cc: Bernd Schmidt, Mark Wielaard, Manuel López-Ibáñez, GCC Patches On Tue, 2015-09-15 at 19:10 +0200, Jakub Jelinek wrote: > On Tue, Sep 15, 2015 at 10:02:15AM -0700, Steve Ellcey wrote: > > I am not sure I like this change. It broke the GLIBC build for me on > > MIPS. Basically GLIBC has a header file with initialized static > > constant globals (sysdeps/ieee754/dbl-64/atnat2.h contains tqpi1 and > > qpi1) and that header file is included in multiple .c files like > > Multiple? All I can see is e_atan2.c including that header file, nothing > else. Whoops, bad assumption on my part. I thought it must be included somewhere else, otherwise why put it in a header. > > sysdeps/ieee754/dbl-64/e_atan2.c that use some, but not all, of those > > static constant variables. But between the various .c files all of the > > globals are used somewhere, just not in every individual .c file. This > > seems like a perfectly reasonable use of static globals and header files > > that should not be identified as a warning. > > I disagree. While const vars are special in C++, it is really like > any other variable in C, so the warning is IMHO appropriate. > > Jakub I guess it is not the 'const' I think should be handled special but the 'static'. Having unused static variables (const or not) declared in a header file but unused seems reasonable since the header file may be included in multiple .c files each of which uses a subset of the static variables. Steve Ellcey sellcey@imgtec.com ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables 2015-09-15 17:26 ` Steve Ellcey @ 2015-09-15 18:55 ` Mark Wielaard 2015-09-15 19:21 ` Mark Wielaard 2015-09-15 19:58 ` Joseph Myers 2015-09-19 2:57 ` Martin Sebor 1 sibling, 2 replies; 43+ messages in thread From: Mark Wielaard @ 2015-09-15 18:55 UTC (permalink / raw) To: sellcey Cc: Jakub Jelinek, Bernd Schmidt, Manuel López-Ibáñez, GCC Patches [-- Attachment #1: Type: text/plain, Size: 1027 bytes --] On Tue, 2015-09-15 at 10:20 -0700, Steve Ellcey wrote: > On Tue, 2015-09-15 at 19:10 +0200, Jakub Jelinek wrote: > > On Tue, Sep 15, 2015 at 10:02:15AM -0700, Steve Ellcey wrote: > > > I am not sure I like this change. It broke the GLIBC build for me on > > > MIPS. Basically GLIBC has a header file with initialized static > > > constant globals (sysdeps/ieee754/dbl-64/atnat2.h contains tqpi1 and > > > qpi1) and that header file is included in multiple .c files like > > > > Multiple? All I can see is e_atan2.c including that header file, nothing > > else. > > Whoops, bad assumption on my part. I thought it must be included > somewhere else, otherwise why put it in a header. Yeah, odd. But it seems the issues the warning pointed out are real. I build glibc and found 9 unused variables. They all look like they are really not used in the code, so they can all just be removed. Someone of course should double check they aren't unused by accident before committing upstream. Cheers, Mark [-- Attachment #2: glibc_unused_vars.patch --] [-- Type: text/x-patch, Size: 5666 bytes --] diff --git a/math/atest-exp2.c b/math/atest-exp2.c index 307c741..ffa73b1 100644 --- a/math/atest-exp2.c +++ b/math/atest-exp2.c @@ -53,11 +53,6 @@ static const mp1 mp_exp1 = { a784d904, 5190cfef, 324e7738, 926cfbe5, f4bf8d8d, 8c31d763) }; -static const mp1 mp_exp_m1 = { - CONSTSZ (0, 5e2d58d8, b3bcdf1a, badec782, 9054f90d, da9805aa, b56c7733, - 3024b9d0, a507daed, b16400bf, 472b4215, b8245b66, 9d90d27a) -}; - static const mp1 mp_log2 = { CONSTSZ (0, b17217f7, d1cf79ab, c9e3b398, 03f2f6af, 40f34326, 7298b62d, 8a0d175b, 8baafa2b, e7b87620, 6debac98, 559552fb, 4afa1b10) diff --git a/resolv/base64.c b/resolv/base64.c index ea584ed..519e5d2 100644 --- a/resolv/base64.c +++ b/resolv/base64.c @@ -40,10 +40,6 @@ * IF IBM IS APPRISED OF THE POSSIBILITY OF SUCH DAMAGES. */ -#if !defined(LINT) && !defined(CODECENTER) -static const char rcsid[] = "$BINDId: base64.c,v 8.7 1999/10/13 16:39:33 vixie Exp $"; -#endif /* not lint */ - #include <sys/types.h> #include <sys/param.h> #include <sys/socket.h> diff --git a/sysdeps/ieee754/dbl-64/atnat2.h b/sysdeps/ieee754/dbl-64/atnat2.h index e0d65af..82943f9 100644 --- a/sysdeps/ieee754/dbl-64/atnat2.h +++ b/sysdeps/ieee754/dbl-64/atnat2.h @@ -65,10 +65,8 @@ /**/ hpi1 = {{0x3c91a626, 0x33145c07} }, /* pi/2-hpi */ /**/ mhpi = {{0xbff921fb, 0x54442d18} }, /* -pi/2 */ /**/ qpi = {{0x3fe921fb, 0x54442d18} }, /* pi/4 */ -/**/ qpi1 = {{0x3c81a626, 0x33145c07} }, /* pi/4-qpi */ /**/ mqpi = {{0xbfe921fb, 0x54442d18} }, /* -pi/4 */ /**/ tqpi = {{0x4002d97c, 0x7f3321d2} }, /* 3pi/4 */ -/**/ tqpi1 = {{0x3c9a7939, 0x4c9e8a0a} }, /* 3pi/4-tqpi */ /**/ mtqpi = {{0xc002d97c, 0x7f3321d2} }, /* -3pi/4 */ /**/ u1 = {{0x3c314c2a, 0x00000000} }, /* 9.377e-19 */ /**/ u2 = {{0x3bf955e4, 0x00000000} }, /* 8.584e-20 */ @@ -129,10 +127,8 @@ /**/ hpi1 = {{0x33145c07, 0x3c91a626} }, /* pi/2-hpi */ /**/ mhpi = {{0x54442d18, 0xbff921fb} }, /* -pi/2 */ /**/ qpi = {{0x54442d18, 0x3fe921fb} }, /* pi/4 */ -/**/ qpi1 = {{0x33145c07, 0x3c81a626} }, /* pi/4-qpi */ /**/ mqpi = {{0x54442d18, 0xbfe921fb} }, /* -pi/4 */ /**/ tqpi = {{0x7f3321d2, 0x4002d97c} }, /* 3pi/4 */ -/**/ tqpi1 = {{0x4c9e8a0a, 0x3c9a7939} }, /* 3pi/4-tqpi */ /**/ mtqpi = {{0x7f3321d2, 0xc002d97c} }, /* -3pi/4 */ /**/ u1 = {{0x00000000, 0x3c314c2a} }, /* 9.377e-19 */ /**/ u2 = {{0x00000000, 0x3bf955e4} }, /* 8.584e-20 */ diff --git a/sysdeps/ieee754/dbl-64/uexp.h b/sysdeps/ieee754/dbl-64/uexp.h index 6817eaf..42b21f2 100644 --- a/sysdeps/ieee754/dbl-64/uexp.h +++ b/sysdeps/ieee754/dbl-64/uexp.h @@ -29,7 +29,7 @@ #include "mydefs.h" -const static double one = 1.0, zero = 0.0, hhuge = 1.0e300, tiny = 1.0e-300, +const static double zero = 0.0, hhuge = 1.0e300, tiny = 1.0e-300, err_0 = 1.000014, err_1 = 0.000016; const static int4 bigint = 0x40862002, badint = 0x40876000,smallint = 0x3C8fffff; diff --git a/sysdeps/ieee754/dbl-64/upow.h b/sysdeps/ieee754/dbl-64/upow.h index c8569a9..b4911e5 100644 --- a/sysdeps/ieee754/dbl-64/upow.h +++ b/sysdeps/ieee754/dbl-64/upow.h @@ -34,7 +34,6 @@ /**/ nZERO = {{0x80000000, 0}}, /* -0.0 */ /**/ INF = {{0x7ff00000, 0x00000000}}, /* INF */ /**/ nINF = {{0xfff00000, 0x00000000}}, /* -INF */ -/**/ sqrt_2 = {{0x3ff6a09e, 0x667f3bcc}}, /* sqrt(2) */ /**/ ln2a = {{0x3fe62e42, 0xfefa3800}}, /* ln(2) 43 bits */ /**/ ln2b = {{0x3d2ef357, 0x93c76730}}, /* ln(2)-ln2a */ /**/ bigu = {{0x4297ffff, 0xfffffd2c}}, /* 1.5*2**42 -724*2**-10 */ @@ -48,7 +47,6 @@ /**/ nZERO = {{0, 0x80000000}}, /* -0.0 */ /**/ INF = {{0x00000000, 0x7ff00000}}, /* INF */ /**/ nINF = {{0x00000000, 0xfff00000}}, /* -INF */ -/**/ sqrt_2 = {{0x667f3bcc, 0x3ff6a09e}}, /* sqrt(2) */ /**/ ln2a = {{0xfefa3800, 0x3fe62e42}}, /* ln(2) 43 bits */ /**/ ln2b = {{0x93c76730, 0x3d2ef357}}, /* ln(2)-ln2a */ /**/ bigu = {{0xfffffd2c, 0x4297ffff}}, /* 1.5*2**42 -724*2**-10 */ diff --git a/sysdeps/ieee754/flt-32/e_log10f.c b/sysdeps/ieee754/flt-32/e_log10f.c index 96f0e81..1daeef7 100644 --- a/sysdeps/ieee754/flt-32/e_log10f.c +++ b/sysdeps/ieee754/flt-32/e_log10f.c @@ -22,8 +22,6 @@ ivln10 = 4.3429449201e-01, /* 0x3ede5bd9 */ log10_2hi = 3.0102920532e-01, /* 0x3e9a2080 */ log10_2lo = 7.9034151668e-07; /* 0x355427db */ -static const float zero = 0.0; - float __ieee754_log10f(float x) { diff --git a/timezone/private.h b/timezone/private.h index 4e8f4ae..ed19e06 100644 --- a/timezone/private.h +++ b/timezone/private.h @@ -326,16 +326,6 @@ const char * scheck(const char * string, const char * format); #define TYPE_SIGNED(type) (((type) -1) < 0) #endif /* !defined TYPE_SIGNED */ -/* The minimum and maximum finite time values. */ -static time_t const time_t_min = - (TYPE_SIGNED(time_t) - ? (time_t) -1 << (CHAR_BIT * sizeof (time_t) - 1) - : 0); -static time_t const time_t_max = - (TYPE_SIGNED(time_t) - ? - (~ 0 < 0) - ((time_t) -1 << (CHAR_BIT * sizeof (time_t) - 1)) - : -1); - #ifndef INT_STRLEN_MAXIMUM /* ** 302 / 1000 is log10(2.0) rounded up. ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables 2015-09-15 18:55 ` Mark Wielaard @ 2015-09-15 19:21 ` Mark Wielaard 2015-09-15 19:58 ` Joseph Myers 1 sibling, 0 replies; 43+ messages in thread From: Mark Wielaard @ 2015-09-15 19:21 UTC (permalink / raw) To: sellcey Cc: Jakub Jelinek, Bernd Schmidt, Manuel López-Ibáñez, GCC Patches On Tue, 2015-09-15 at 20:33 +0200, Mark Wielaard wrote: > I build glibc and found 9 unused variables. They all look like they are > really not used in the code, so they can all just be removed. Someone of > course should double check they aren't unused by accident before > committing upstream. For the record there was one other issue with building glibc with current GCC6 was: tst-endian.c: In function ‘do_test’: tst-endian.c:22:30: error: self-comparison always evaluates to false [-Werror=tautological-compare] if (htole16 (le16toh (i)) != i) ^ which is fixed by: diff --git a/string/tst-endian.c b/string/tst-endian.c index 8684bb2..9d64a87 100644 --- a/string/tst-endian.c +++ b/string/tst-endian.c @@ -9,7 +9,7 @@ do_test (void) { int result = 0; - for (uint64_t i = 0; i < (~UINT64_C (0)) >> 2; i = (i << 1) + 3) + for (volatile uint64_t i = 0; i < (~UINT64_C (0)) >> 2; i = (i << 1) + 3) { if (i < UINT64_C (65536)) { ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables 2015-09-15 18:55 ` Mark Wielaard 2015-09-15 19:21 ` Mark Wielaard @ 2015-09-15 19:58 ` Joseph Myers 1 sibling, 0 replies; 43+ messages in thread From: Joseph Myers @ 2015-09-15 19:58 UTC (permalink / raw) To: Mark Wielaard Cc: sellcey, Jakub Jelinek, Bernd Schmidt, Manuel López-Ibáñez, GCC Patches timezone/private.h comes verbatim from the tzcode project and should not be modified locally in glibc. -- Joseph S. Myers joseph@codesourcery.com ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables 2015-09-15 17:26 ` Steve Ellcey 2015-09-15 18:55 ` Mark Wielaard @ 2015-09-19 2:57 ` Martin Sebor 2015-09-21 17:10 ` Steve Ellcey 2015-09-23 18:26 ` Jeff Law 1 sibling, 2 replies; 43+ messages in thread From: Martin Sebor @ 2015-09-19 2:57 UTC (permalink / raw) To: sellcey, Jakub Jelinek Cc: Bernd Schmidt, Mark Wielaard, Manuel López-Ibáñez, GCC Patches On 09/15/2015 11:20 AM, Steve Ellcey wrote: > On Tue, 2015-09-15 at 19:10 +0200, Jakub Jelinek wrote: >> On Tue, Sep 15, 2015 at 10:02:15AM -0700, Steve Ellcey wrote: >>> I am not sure I like this change. It broke the GLIBC build for me on >>> MIPS. Basically GLIBC has a header file with initialized static >>> constant globals (sysdeps/ieee754/dbl-64/atnat2.h contains tqpi1 and >>> qpi1) and that header file is included in multiple .c files like >> >> Multiple? All I can see is e_atan2.c including that header file, nothing >> else. > > Whoops, bad assumption on my part. I thought it must be included > somewhere else, otherwise why put it in a header. > >>> sysdeps/ieee754/dbl-64/e_atan2.c that use some, but not all, of those >>> static constant variables. But between the various .c files all of the >>> globals are used somewhere, just not in every individual .c file. This >>> seems like a perfectly reasonable use of static globals and header files >>> that should not be identified as a warning. >> >> I disagree. While const vars are special in C++, it is really like >> any other variable in C, so the warning is IMHO appropriate. >> >> Jakub > > I guess it is not the 'const' I think should be handled special but the > 'static'. Having unused static variables (const or not) declared in a > header file but unused seems reasonable since the header file may be > included in multiple .c files each of which uses a subset of the static > variables. I tend to agree. I suppose diagnosing unused non-const static definitions might be helpful but I can't think of a good reason to diagnose unused initialized static consts in C. Especially since they're not diagnosed in C++. Would diagnosing them in source files while avoiding the warning for static const definitions in headers be an acceptable compromise? Martin ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables 2015-09-19 2:57 ` Martin Sebor @ 2015-09-21 17:10 ` Steve Ellcey 2015-09-23 18:26 ` Jeff Law 1 sibling, 0 replies; 43+ messages in thread From: Steve Ellcey @ 2015-09-21 17:10 UTC (permalink / raw) To: Martin Sebor Cc: Jakub Jelinek, Bernd Schmidt, Mark Wielaard, Manuel López-Ibáñez, GCC Patches On Fri, 2015-09-18 at 20:29 -0600, Martin Sebor wrote: > On 09/15/2015 11:20 AM, Steve Ellcey wrote: > > I guess it is not the 'const' I think should be handled special but the > > 'static'. Having unused static variables (const or not) declared in a > > header file but unused seems reasonable since the header file may be > > included in multiple .c files each of which uses a subset of the static > > variables. > > I tend to agree. I suppose diagnosing unused non-const static > definitions might be helpful but I can't think of a good reason > to diagnose unused initialized static consts in C. Especially > since they're not diagnosed in C++. > > Would diagnosing them in source files while avoiding the warning > for static const definitions in headers be an acceptable compromise? > > Martin That seems like a reasonable compromise to me. Steve Ellcey sellcey@imgtec.com ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables 2015-09-19 2:57 ` Martin Sebor 2015-09-21 17:10 ` Steve Ellcey @ 2015-09-23 18:26 ` Jeff Law 2015-09-24 12:54 ` Mark Wielaard 1 sibling, 1 reply; 43+ messages in thread From: Jeff Law @ 2015-09-23 18:26 UTC (permalink / raw) To: Martin Sebor, sellcey, Jakub Jelinek Cc: Bernd Schmidt, Mark Wielaard, Manuel López-Ibáñez, GCC Patches On 09/18/2015 08:29 PM, Martin Sebor wrote: >> I guess it is not the 'const' I think should be handled special but the >> 'static'. Having unused static variables (const or not) declared in a >> header file but unused seems reasonable since the header file may be >> included in multiple .c files each of which uses a subset of the static >> variables. > > I tend to agree. I suppose diagnosing unused non-const static > definitions might be helpful but I can't think of a good reason > to diagnose unused initialized static consts in C. Especially > since they're not diagnosed in C++. > > Would diagnosing them in source files while avoiding the warning > for static const definitions in headers be an acceptable compromise? It's probably worth a try. jeff ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables 2015-09-23 18:26 ` Jeff Law @ 2015-09-24 12:54 ` Mark Wielaard 2015-09-24 13:06 ` Bernd Schmidt 0 siblings, 1 reply; 43+ messages in thread From: Mark Wielaard @ 2015-09-24 12:54 UTC (permalink / raw) To: Jeff Law Cc: Martin Sebor, sellcey, Jakub Jelinek, Bernd Schmidt, Manuel López-Ibáñez, GCC Patches On Wed, 2015-09-23 at 12:25 -0600, Jeff Law wrote: > On 09/18/2015 08:29 PM, Martin Sebor wrote: > >> I guess it is not the 'const' I think should be handled special but the > >> 'static'. Having unused static variables (const or not) declared in a > >> header file but unused seems reasonable since the header file may be > >> included in multiple .c files each of which uses a subset of the static > >> variables. > > > > I tend to agree. I suppose diagnosing unused non-const static > > definitions might be helpful but I can't think of a good reason > > to diagnose unused initialized static consts in C. Especially > > since they're not diagnosed in C++. > > > > Would diagnosing them in source files while avoiding the warning > > for static const definitions in headers be an acceptable compromise? > It's probably worth a try. I am a little concerned that would hide some real issues. In the case of glibc the header files were actually only used by one main file, so the variables were indeed unused and needed to be investigated why they were there in the first place. In the case of wine the issue was that the header file contained non-idiomatic and somewhat unreadable C constructs that could easily be replaced by more readable defines for 16bit char string constants. Even if there are such constructs in header files and they aren't actually bugs or people are unwilling to fix the issue with something that is more idiomatic C then there are various ways to suppress the warning. Either just don't use -Wunused-variable or add -Wno-unused-const-variable. Add an explicit __attribute__((used)) or just add a #pragma GCC system_header to the .h file. If we are concerned that this generates warnings that aren't easy to avoid then we might want to add that particular check behind -Wextra. But is that really necessary? I am not against implementing an extra warning exception/flag if it really is necessary. But it does introduce more complexity and makes the warning less consistent. So what would be a good way to find out one way or another whether the extra complexity is needed? Cheers, Mark ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables 2015-09-24 12:54 ` Mark Wielaard @ 2015-09-24 13:06 ` Bernd Schmidt 2015-09-24 16:38 ` Steve Ellcey 2016-02-21 1:43 ` [PATCH] PR28901 Add two levels for -Wunused-const-variable Mark Wielaard 0 siblings, 2 replies; 43+ messages in thread From: Bernd Schmidt @ 2015-09-24 13:06 UTC (permalink / raw) To: Mark Wielaard, Jeff Law Cc: Martin Sebor, sellcey, Jakub Jelinek, Manuel López-Ibáñez, GCC Patches On 09/24/2015 01:53 PM, Mark Wielaard wrote: > Even if there are such constructs in header files and they aren't > actually bugs or people are unwilling to fix the issue with something > that is more idiomatic C then there are various ways to suppress the > warning. Either just don't use -Wunused-variable or add > -Wno-unused-const-variable. Add an explicit __attribute__((used)) or > just add a #pragma GCC system_header to the .h file. > > If we are concerned that this generates warnings that aren't easy to > avoid then we might want to add that particular check behind -Wextra. > But is that really necessary? I am not against implementing an extra > warning exception/flag if it really is necessary. But it does introduce > more complexity and makes the warning less consistent. So what would be > a good way to find out one way or another whether the extra complexity > is needed? I think at this point we have reports of just two packages generating extra warnings, with the warnings at least justifiable in both cases. So my vote would be to leave things as-is for now and see if more reports come in. It is after all expected that a new warning option generates new warnings. Bernd ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables 2015-09-24 13:06 ` Bernd Schmidt @ 2015-09-24 16:38 ` Steve Ellcey 2015-09-24 18:40 ` Bernd Schmidt 2016-02-21 1:43 ` [PATCH] PR28901 Add two levels for -Wunused-const-variable Mark Wielaard 1 sibling, 1 reply; 43+ messages in thread From: Steve Ellcey @ 2015-09-24 16:38 UTC (permalink / raw) To: Bernd Schmidt Cc: Mark Wielaard, Jeff Law, Martin Sebor, Jakub Jelinek, Manuel López-Ibáñez, GCC Patches On Thu, 2015-09-24 at 13:56 +0200, Bernd Schmidt wrote: > I think at this point we have reports of just two packages generating > extra warnings, with the warnings at least justifiable in both cases. So > my vote would be to leave things as-is for now and see if more reports > come in. It is after all expected that a new warning option generates > new warnings. > > > Bernd At least one of the warnings in glibc is not justified (in my opinion). The header file timezone/private.h defines time_t_min and time_t_max. These are not used in any of the timezone files built by glibc but if you look at the complete tz package they are used when building other objects that are not part of the glibc tz component and that include private.h. I would make two arguments about why I don't think we should warn. One is that 'static int const foo = 1' seems a lot like '#define foo 1' and we don't complain about the macro foo not being used. If we complain about the unused const, why not complain about the unused macro? We don't complain because we know it would result in too many warnings in existing code. If we want people to move away from macros, and I think we do, then we should not make it harder to do so by introducing new warnings when they change. The other is that C++ does not complain about this. I know that C and C++ are different languages with different rules but it seems like this difference is a difference that doesn't have to exist. Either both should complain or neither should complain. I can't think of any valid reason for one to complain and the other not to. I think using the used attribute is probably a reasonable way to address this issue if we continue to generate the warning but I still feel it is a bad warning in that it will (sometimes) warn about a coding style that seems perfectly reasonable to me. Steve Ellcey sellcey@imgtec.com ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables 2015-09-24 16:38 ` Steve Ellcey @ 2015-09-24 18:40 ` Bernd Schmidt 2015-09-25 8:00 ` Trevor Saunders 0 siblings, 1 reply; 43+ messages in thread From: Bernd Schmidt @ 2015-09-24 18:40 UTC (permalink / raw) To: sellcey Cc: Mark Wielaard, Jeff Law, Martin Sebor, Jakub Jelinek, Manuel López-Ibáñez, GCC Patches On 09/24/2015 06:11 PM, Steve Ellcey wrote: > At least one of the warnings in glibc is not justified (in my opinion). > The header file timezone/private.h defines time_t_min and time_t_max. > These are not used in any of the timezone files built by glibc but if > you look at the complete tz package they are used when building other > objects that are not part of the glibc tz component and that include > private.h. The standard C way of writing this would be to declare time_t_min in the header and have its definition in another file, or use a TIME_T_MIN macro as glibc does in mktime.c. That file even has a local redefinition: time_t time_t_min = TIME_T_MIN; So at the very least the warning points at code that has some oddities. > I would make two arguments about why I don't think we should warn. > > One is that 'static int const foo = 1' seems a lot like '#define foo 1' > and we don't complain about the macro foo not being used. If we > complain about the unused const, why not complain about the unused > macro? We don't complain because we know it would result in too many > warnings in existing code. If we want people to move away from macros, > and I think we do, then we should not make it harder to do so by > introducing new warnings when they change. > > The other is that C++ does not complain about this. I know that C and > C++ are different languages with different rules but it seems like this > difference is a difference that doesn't have to exist. Either both > should complain or neither should complain. I can't think of any valid > reason for one to complain and the other not to. Well, they _are_ different languages, and handling of const is one place where they differ. For example, C++ consts can be used in places where constant expressions are required. The following is a valid C++ program but not a C program: const int v = 200; int t[v]; The result is that the typical programming style for C is to have constants #defined, while for C++ you can find more examples like the above; I recall Stroustrup explicitly advocating that in the introductory books I read 20 years ago, and using it as a selling point for C++. Existing practice is important when deciding what to warn about, and for the moment I remain convinced that C practice is sufficiently different from C++. Bernd ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables 2015-09-24 18:40 ` Bernd Schmidt @ 2015-09-25 8:00 ` Trevor Saunders 2015-10-06 22:44 ` Steve Ellcey 0 siblings, 1 reply; 43+ messages in thread From: Trevor Saunders @ 2015-09-25 8:00 UTC (permalink / raw) To: Bernd Schmidt Cc: sellcey, Mark Wielaard, Jeff Law, Martin Sebor, Jakub Jelinek, Manuel López-Ibáñez, GCC Patches On Thu, Sep 24, 2015 at 06:55:11PM +0200, Bernd Schmidt wrote: > On 09/24/2015 06:11 PM, Steve Ellcey wrote: > >At least one of the warnings in glibc is not justified (in my opinion). > >The header file timezone/private.h defines time_t_min and time_t_max. > >These are not used in any of the timezone files built by glibc but if > >you look at the complete tz package they are used when building other > >objects that are not part of the glibc tz component and that include > >private.h. > > The standard C way of writing this would be to declare time_t_min in the > header and have its definition in another file, or use a TIME_T_MIN macro as > glibc does in mktime.c. That file even has a local redefinition: > time_t time_t_min = TIME_T_MIN; > So at the very least the warning points at code that has some oddities. I can believe its an odd way to write C, but is it actually a bad one? I expect if I got warnings for code like that I'd be pretty unhappy about either moving the constant out where the compiler can't always see it, or making it a macro. > >I would make two arguments about why I don't think we should warn. > > > >One is that 'static int const foo = 1' seems a lot like '#define foo 1' > >and we don't complain about the macro foo not being used. If we > >complain about the unused const, why not complain about the unused > >macro? We don't complain because we know it would result in too many > >warnings in existing code. If we want people to move away from macros, > >and I think we do, then we should not make it harder to do so by > >introducing new warnings when they change. > > > >The other is that C++ does not complain about this. I know that C and > >C++ are different languages with different rules but it seems like this > >difference is a difference that doesn't have to exist. Either both > >should complain or neither should complain. I can't think of any valid > >reason for one to complain and the other not to. > > Well, they _are_ different languages, and handling of const is one place > where they differ. For example, C++ consts can be used in places where > constant expressions are required. The following is a valid C++ program but > not a C program: > > const int v = 200; > int t[v]; > > The result is that the typical programming style for C is to have constants > #defined, while for C++ you can find more examples like the above; I recall > Stroustrup explicitly advocating that in the introductory books I read 20 > years ago, and using it as a selling point for C++. Existing practice is > important when deciding what to warn about, and for the moment I remain > convinced that C practice is sufficiently different from C++. existing practice is certainly important, but I would say that what is good practice is also very important. It seems to me that warning for these constants is basically making it hard to follow a better practice than the existing one. That seems pretty unfortunate. On the other hand I've become much more of a C++ programmer than a C one so, I'm probably not the best judge. Trev > > > Bernd ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables 2015-09-25 8:00 ` Trevor Saunders @ 2015-10-06 22:44 ` Steve Ellcey 2015-10-07 12:00 ` Bernd Schmidt 0 siblings, 1 reply; 43+ messages in thread From: Steve Ellcey @ 2015-10-06 22:44 UTC (permalink / raw) To: Trevor Saunders Cc: Bernd Schmidt, Mark Wielaard, Jeff Law, Martin Sebor, Jakub Jelinek, Manuel López-Ibáñez, GCC Patches On Thu, 2015-09-24 at 21:24 -0400, Trevor Saunders wrote: > On Thu, Sep 24, 2015 at 06:55:11PM +0200, Bernd Schmidt wrote: > > On 09/24/2015 06:11 PM, Steve Ellcey wrote: > > >At least one of the warnings in glibc is not justified (in my opinion). > > >The header file timezone/private.h defines time_t_min and time_t_max. > > >These are not used in any of the timezone files built by glibc but if > > >you look at the complete tz package they are used when building other > > >objects that are not part of the glibc tz component and that include > > >private.h. > > > > The standard C way of writing this would be to declare time_t_min in the > > header and have its definition in another file, or use a TIME_T_MIN macro as > > glibc does in mktime.c. That file even has a local redefinition: > > time_t time_t_min = TIME_T_MIN; > > So at the very least the warning points at code that has some oddities. > > I can believe its an odd way to write C, but is it actually a bad one? > I expect if I got warnings for code like that I'd be pretty unhappy > about either moving the constant out where the compiler can't always see > it, or making it a macro. > > > >I would make two arguments about why I don't think we should warn. > > > > > >One is that 'static int const foo = 1' seems a lot like '#define foo 1' > > >and we don't complain about the macro foo not being used. If we > > >complain about the unused const, why not complain about the unused > > >macro? We don't complain because we know it would result in too many > > >warnings in existing code. If we want people to move away from macros, > > >and I think we do, then we should not make it harder to do so by > > >introducing new warnings when they change. > > > > > >The other is that C++ does not complain about this. I know that C and > > >C++ are different languages with different rules but it seems like this > > >difference is a difference that doesn't have to exist. Either both > > >should complain or neither should complain. I can't think of any valid > > >reason for one to complain and the other not to. > > > > Well, they _are_ different languages, and handling of const is one place > > where they differ. For example, C++ consts can be used in places where > > constant expressions are required. The following is a valid C++ program but > > not a C program: > > > > const int v = 200; > > int t[v]; > > > > The result is that the typical programming style for C is to have constants > > #defined, while for C++ you can find more examples like the above; I recall > > Stroustrup explicitly advocating that in the introductory books I read 20 > > years ago, and using it as a selling point for C++. Existing practice is > > important when deciding what to warn about, and for the moment I remain > > convinced that C practice is sufficiently different from C++. > > existing practice is certainly important, but I would say that what is > good practice is also very important. It seems to me that warning for > these constants is basically making it hard to follow a better practice > than the existing one. That seems pretty unfortunate. > > On the other hand I've become much more of a C++ programmer than a C one > so, I'm probably not the best judge. > > Trev > > > > > > > Bernd So, is there any consensus on this issue? I cannot build top-of-tree glibc with top-of-tree GCC due to this warning coming from timezone/private.h. If GCC is going to keep this warning then I should talk to the owners of the header file (tz group, not glibc) and see if they would be willing to add a unused attribute to it. Personally, I would rather have GCC not issue the warning, but that is just based on my opinion that the definition of a (possibly) unused static initialized variable in a C header is a reasonable practice. Steve Ellcey sellcey@imgtec.com ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables 2015-10-06 22:44 ` Steve Ellcey @ 2015-10-07 12:00 ` Bernd Schmidt 2015-10-24 15:00 ` Gerald Pfeifer 0 siblings, 1 reply; 43+ messages in thread From: Bernd Schmidt @ 2015-10-07 12:00 UTC (permalink / raw) To: sellcey, Trevor Saunders Cc: Mark Wielaard, Jeff Law, Martin Sebor, Jakub Jelinek, Manuel López-Ibáñez, GCC Patches On 10/07/2015 12:44 AM, Steve Ellcey wrote: > So, is there any consensus on this issue? If I understood Jakub correctly he agreed with my reasoning. So for the moment we'll leave things as they are and revisit the issue if and when other cases pop up. > I cannot build top-of-tree > glibc with top-of-tree GCC due to this warning coming from > timezone/private.h. If GCC is going to keep this warning then I should > talk to the owners of the header file (tz group, not glibc) and see if > they would be willing to add a unused attribute to it. I think not using -Wall -Werror is the right fix. Bernd ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables 2015-10-07 12:00 ` Bernd Schmidt @ 2015-10-24 15:00 ` Gerald Pfeifer 2015-10-24 15:11 ` Mark Wielaard 0 siblings, 1 reply; 43+ messages in thread From: Gerald Pfeifer @ 2015-10-24 15:00 UTC (permalink / raw) To: Bernd Schmidt Cc: sellcey, Trevor Saunders, Mark Wielaard, Jeff Law, Martin Sebor, Jakub Jelinek, Manuel López-Ibáñez, GCC Patches On Wed, 7 Oct 2015, Bernd Schmidt wrote: > I think not using -Wall -Werror is the right fix. Of course there is a fair chance (and I'll likely end up proposing this for Wine) is that people will use -Wnounused-variable instead. Which will disable _all_ such warnings, not only those coming from const ... ... = ...; in C header files. Gerald ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables 2015-10-24 15:00 ` Gerald Pfeifer @ 2015-10-24 15:11 ` Mark Wielaard 0 siblings, 0 replies; 43+ messages in thread From: Mark Wielaard @ 2015-10-24 15:11 UTC (permalink / raw) To: Gerald Pfeifer Cc: Bernd Schmidt, sellcey, Trevor Saunders, Jeff Law, Martin Sebor, Jakub Jelinek, Manuel López-Ibáñez, GCC Patches On Sat, 2015-10-24 at 15:47 +0200, Gerald Pfeifer wrote: > On Wed, 7 Oct 2015, Bernd Schmidt wrote: > > I think not using -Wall -Werror is the right fix. > > Of course there is a fair chance (and I'll likely end up proposing > this for Wine) is that people will use -Wnounused-variable instead. > > Which will disable _all_ such warnings, not only those coming from > const ... ... = ...; Then use -Wno-unused-const-variable, not -Wno-unused-variable, if you only want to suppress the new warning. Cheers, Mark ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH] PR28901 Add two levels for -Wunused-const-variable. 2015-09-24 13:06 ` Bernd Schmidt 2015-09-24 16:38 ` Steve Ellcey @ 2016-02-21 1:43 ` Mark Wielaard 2016-02-22 18:58 ` Jeff Law 1 sibling, 1 reply; 43+ messages in thread From: Mark Wielaard @ 2016-02-21 1:43 UTC (permalink / raw) To: gcc-patches Cc: Bernd Schmidt, Jeff Law, Martin Sebor, sellcey, Jakub Jelinek, Manuel López-Ibáñez, Mark Wielaard There is some controversy about enabling -Wunused-const-variable for all unused static const variables because some feel there are too many errors exposed in header files. Create two levels for -Wunused-const-variable. One level to only check for unused static const variables in the main compilation file. Which is enabled by -Wunused-variable. And a second level that also checks for unused static const variables in included header files. Which must be explicitly enabled. gcc/ChangeLog PR c/28901 * cgraphunit.c (check_global_declaration): Check level of warn_unused_const_variable and main_input_filename. * doc/invoke.texi (Warning Options): Add -Wunused-const-variable=. (-Wunused-variable): For C implies -Wunused-const-variable=1. (-Wunused-const-variable): Explain levels 1 and 2. gcc/c-family/ChangeLog PR c/28901 * c.opt (Wunused-const-variable): Turn into Alias for... (Wunused-const-variable=): New option. gcc/testsuite/ChangeLog PR c/28901 * gcc.dg/unused-variable-3.c: New test. --- diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt index 638e9c2..7c5f6c7 100644 --- a/gcc/c-family/c.opt +++ b/gcc/c-family/c.opt @@ -949,7 +949,11 @@ C ObjC C++ ObjC++ LangEnabledBy(C ObjC C++ ObjC++,Wunused) ; documented in common.opt Wunused-const-variable -C ObjC C++ ObjC++ Var(warn_unused_const_variable) Warning LangEnabledBy(C ObjC,Wunused-variable) +C ObjC C++ ObjC++ Warning Alias(Wunused-const-variable=, 2, 0) +Warn when a const variable is unused. + +Wunused-const-variable= +C ObjC C++ ObjC++ Joined RejectNegative UInteger Var(warn_unused_const_variable) Warning LangEnabledBy(C ObjC,Wunused-variable, 1, 0) Warn when a const variable is unused. Wvariadic-macros diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c index 0a745f0..27a073a 100644 --- a/gcc/cgraphunit.c +++ b/gcc/cgraphunit.c @@ -942,7 +942,10 @@ check_global_declaration (symtab_node *snode) /* Warn about static fns or vars defined but not used. */ if (((warn_unused_function && TREE_CODE (decl) == FUNCTION_DECL) || (((warn_unused_variable && ! TREE_READONLY (decl)) - || (warn_unused_const_variable && TREE_READONLY (decl))) + || (warn_unused_const_variable > 0 && TREE_READONLY (decl) + && (warn_unused_const_variable == 2 + || filename_cmp (main_input_filename, + DECL_SOURCE_FILE (decl)) == 0))) && TREE_CODE (decl) == VAR_DECL)) && ! DECL_IN_SYSTEM_HEADER (decl) && ! snode->referred_to_p (/*include_self=*/false) @@ -971,7 +974,7 @@ check_global_declaration (symtab_node *snode) (TREE_CODE (decl) == FUNCTION_DECL) ? OPT_Wunused_function : (TREE_READONLY (decl) - ? OPT_Wunused_const_variable + ? OPT_Wunused_const_variable_ : OPT_Wunused_variable), "%qD defined but not used", decl); } diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index c1ab788..490df93 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -303,7 +303,7 @@ Objective-C and Objective-C++ Dialects}. -Wunsuffixed-float-constants -Wunused -Wunused-function @gol -Wunused-label -Wunused-local-typedefs -Wunused-parameter @gol -Wno-unused-result -Wunused-value @gol -Wunused-variable @gol --Wunused-const-variable @gol +-Wunused-const-variable -Wunused-const-variable=@var{n} @gol -Wunused-but-set-parameter -Wunused-but-set-variable @gol -Wuseless-cast -Wvariadic-macros -Wvector-operation-performance @gol -Wvla -Wvolatile-register-var -Wwrite-strings @gol @@ -4231,23 +4231,39 @@ its return value. The default is @option{-Wunused-result}. @opindex Wunused-variable @opindex Wno-unused-variable Warn whenever a local or static variable is unused aside from its -declaration. This option implies @option{-Wunused-const-variable} for C, +declaration. This option implies @option{-Wunused-const-variable=1} for C, but not for C++. This warning is enabled by @option{-Wall}. To suppress this warning use the @code{unused} attribute (@pxref{Variable Attributes}). @item -Wunused-const-variable +@itemx -Wunused-const-variable=@var{n} @opindex Wunused-const-variable @opindex Wno-unused-const-variable Warn whenever a constant static variable is unused aside from its declaration. -This warning is enabled by @option{-Wunused-variable} for C, but not for C++. -In C++ this is normally not an error since const variables take the place of -@code{#define}s in C++. +@option{-Wunused-const-variable=1} is enabled by @option{-Wunused-variable} +for C, but not for C++. In C this declares variable storage, but in C++ this +is not an error since const variables take the place of @code{#define}s. To suppress this warning use the @code{unused} attribute (@pxref{Variable Attributes}). +@table @gcctabopt +@item -Wunused-const-variable=1 +This is the warning level that is enabled by @option{-Wunused-variable} for +C. It warns only about unused static const variables defined in the main +compilation unit, but not about static const variables declared in any +header included. + +@item -Wunused-const-variable=2 +This warning level also warns for unused constant static variables in +headers (excluding system headers). This is the warning level of +@option{-Wunused-const-variable} and must be explicitly requested since +in C++ this isn't an error and in C it might be harder to clean up all +headers included. +@end table + @item -Wunused-value @opindex Wunused-value @opindex Wno-unused-value diff --git a/gcc/testsuite/gcc.dg/unused-variable-3.c b/gcc/testsuite/gcc.dg/unused-variable-3.c new file mode 100644 index 0000000..6aca958 --- /dev/null +++ b/gcc/testsuite/gcc.dg/unused-variable-3.c @@ -0,0 +1,9 @@ +/* { dg-do compile } */ +/* { dg-options "-Wunused-variable" } */ + +static const int cmain = 42; /* { dg-warning "defined but not used" } */ + +/* Don't warn for unused static consts in headers, + unless -Wunused-const-variable=2. */ +#line 1 "header.h" +static const int cheader = 42; -- 1.8.3.1 ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] PR28901 Add two levels for -Wunused-const-variable. 2016-02-21 1:43 ` [PATCH] PR28901 Add two levels for -Wunused-const-variable Mark Wielaard @ 2016-02-22 18:58 ` Jeff Law 2016-02-22 19:00 ` Jakub Jelinek 2016-02-22 22:43 ` Mark Wielaard 0 siblings, 2 replies; 43+ messages in thread From: Jeff Law @ 2016-02-22 18:58 UTC (permalink / raw) To: Mark Wielaard, gcc-patches Cc: Bernd Schmidt, Martin Sebor, sellcey, Jakub Jelinek, Manuel López-Ibáñez On 02/20/2016 06:42 PM, Mark Wielaard wrote: > There is some controversy about enabling -Wunused-const-variable for all > unused static const variables because some feel there are too many errors > exposed in header files. Create two levels for -Wunused-const-variable. > One level to only check for unused static const variables in the main > compilation file. Which is enabled by -Wunused-variable. And a second > level that also checks for unused static const variables in included > header files. Which must be explicitly enabled. > > gcc/ChangeLog > > PR c/28901 > * cgraphunit.c (check_global_declaration): Check level of > warn_unused_const_variable and main_input_filename. > * doc/invoke.texi (Warning Options): Add -Wunused-const-variable=. > (-Wunused-variable): For C implies -Wunused-const-variable=1. > (-Wunused-const-variable): Explain levels 1 and 2. > > gcc/c-family/ChangeLog > > PR c/28901 > * c.opt (Wunused-const-variable): Turn into Alias for... > (Wunused-const-variable=): New option. > > gcc/testsuite/ChangeLog > > PR c/28901 > * gcc.dg/unused-variable-3.c: New test. Note that given the discussion in the BZ, I'm going to consider this a regression and thus eligible for the trunk. > diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c > index 0a745f0..27a073a 100644 > --- a/gcc/cgraphunit.c > +++ b/gcc/cgraphunit.c > @@ -971,7 +974,7 @@ check_global_declaration (symtab_node *snode) > (TREE_CODE (decl) == FUNCTION_DECL) > ? OPT_Wunused_function > : (TREE_READONLY (decl) > - ? OPT_Wunused_const_variable > + ? OPT_Wunused_const_variable_ Typo here? If that's not a typo, then just say so and this is approved. If it is a typo, the patch is approved with the typo fixed appropriately. jeff ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] PR28901 Add two levels for -Wunused-const-variable. 2016-02-22 18:58 ` Jeff Law @ 2016-02-22 19:00 ` Jakub Jelinek 2016-02-22 19:02 ` Jeff Law 2016-02-22 22:43 ` Mark Wielaard 1 sibling, 1 reply; 43+ messages in thread From: Jakub Jelinek @ 2016-02-22 19:00 UTC (permalink / raw) To: Jeff Law Cc: Mark Wielaard, gcc-patches, Bernd Schmidt, Martin Sebor, sellcey, Manuel López-Ibáñez On Mon, Feb 22, 2016 at 11:57:56AM -0700, Jeff Law wrote: > >diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c > >index 0a745f0..27a073a 100644 > >--- a/gcc/cgraphunit.c > >+++ b/gcc/cgraphunit.c > >@@ -971,7 +974,7 @@ check_global_declaration (symtab_node *snode) > > (TREE_CODE (decl) == FUNCTION_DECL) > > ? OPT_Wunused_function > > : (TREE_READONLY (decl) > >- ? OPT_Wunused_const_variable > >+ ? OPT_Wunused_const_variable_ > Typo here? The _ stands for = from the option name, as -Wunused-const-variable is an alias to -Wunused-const-variable=2, I think we want the trailing underscore. Jakub ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] PR28901 Add two levels for -Wunused-const-variable. 2016-02-22 19:00 ` Jakub Jelinek @ 2016-02-22 19:02 ` Jeff Law 0 siblings, 0 replies; 43+ messages in thread From: Jeff Law @ 2016-02-22 19:02 UTC (permalink / raw) To: Jakub Jelinek Cc: Mark Wielaard, gcc-patches, Bernd Schmidt, Martin Sebor, sellcey, Manuel López-Ibáñez On 02/22/2016 12:00 PM, Jakub Jelinek wrote: > On Mon, Feb 22, 2016 at 11:57:56AM -0700, Jeff Law wrote: >>> diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c >>> index 0a745f0..27a073a 100644 >>> --- a/gcc/cgraphunit.c >>> +++ b/gcc/cgraphunit.c >>> @@ -971,7 +974,7 @@ check_global_declaration (symtab_node *snode) >>> (TREE_CODE (decl) == FUNCTION_DECL) >>> ? OPT_Wunused_function >>> : (TREE_READONLY (decl) >>> - ? OPT_Wunused_const_variable >>> + ? OPT_Wunused_const_variable_ >> Typo here? > > The _ stands for = from the option name, as -Wunused-const-variable > is an alias to -Wunused-const-variable=2, I think we want the trailing > underscore. Looked funny. If that's standard practice, then it's fine with me. jeff ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] PR28901 Add two levels for -Wunused-const-variable. 2016-02-22 18:58 ` Jeff Law 2016-02-22 19:00 ` Jakub Jelinek @ 2016-02-22 22:43 ` Mark Wielaard 2016-02-23 3:21 ` H.J. Lu 1 sibling, 1 reply; 43+ messages in thread From: Mark Wielaard @ 2016-02-22 22:43 UTC (permalink / raw) To: Jeff Law Cc: gcc-patches, Bernd Schmidt, Martin Sebor, sellcey, Jakub Jelinek, Manuel López-Ibáñez On Mon, Feb 22, 2016 at 11:57:56AM -0700, Jeff Law wrote: > On 02/20/2016 06:42 PM, Mark Wielaard wrote: > Note that given the discussion in the BZ, I'm going to consider this a > regression and thus eligible for the trunk. Thanks. Unfortunately new warnings always seem to make some people unhappy (even when others are happy and see them as useful). Hopefully this compromise makes it so that nobody sees this warning as regression. > >diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c > >index 0a745f0..27a073a 100644 > >--- a/gcc/cgraphunit.c > >+++ b/gcc/cgraphunit.c > >@@ -971,7 +974,7 @@ check_global_declaration (symtab_node *snode) > > (TREE_CODE (decl) == FUNCTION_DECL) > > ? OPT_Wunused_function > > : (TREE_READONLY (decl) > >- ? OPT_Wunused_const_variable > >+ ? OPT_Wunused_const_variable_ > Typo here? > > If that's not a typo, then just say so and this is approved. As Jakub already explained that was deliberate. It is how a warning option that can take a level is represented. Pushed. Thanks, Mark ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] PR28901 Add two levels for -Wunused-const-variable. 2016-02-22 22:43 ` Mark Wielaard @ 2016-02-23 3:21 ` H.J. Lu 2016-02-23 7:55 ` Mark Wielaard 0 siblings, 1 reply; 43+ messages in thread From: H.J. Lu @ 2016-02-23 3:21 UTC (permalink / raw) To: Mark Wielaard Cc: Jeff Law, GCC Patches, Bernd Schmidt, Martin Sebor, Steve Ellcey, Jakub Jelinek, Manuel López-Ibáñez On Mon, Feb 22, 2016 at 2:43 PM, Mark Wielaard <mjw@redhat.com> wrote: > On Mon, Feb 22, 2016 at 11:57:56AM -0700, Jeff Law wrote: >> On 02/20/2016 06:42 PM, Mark Wielaard wrote: >> Note that given the discussion in the BZ, I'm going to consider this a >> regression and thus eligible for the trunk. > > Thanks. Unfortunately new warnings always seem to make some people > unhappy (even when others are happy and see them as useful). Hopefully > this compromise makes it so that nobody sees this warning as regression. > >> >diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c >> >index 0a745f0..27a073a 100644 >> >--- a/gcc/cgraphunit.c >> >+++ b/gcc/cgraphunit.c >> >@@ -971,7 +974,7 @@ check_global_declaration (symtab_node *snode) >> > (TREE_CODE (decl) == FUNCTION_DECL) >> > ? OPT_Wunused_function >> > : (TREE_READONLY (decl) >> >- ? OPT_Wunused_const_variable >> >+ ? OPT_Wunused_const_variable_ >> Typo here? >> >> If that's not a typo, then just say so and this is approved. > > As Jakub already explained that was deliberate. It is how a warning > option that can take a level is represented. Pushed. > It caused: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69911 -- H.J. ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] PR28901 Add two levels for -Wunused-const-variable. 2016-02-23 3:21 ` H.J. Lu @ 2016-02-23 7:55 ` Mark Wielaard 2016-02-23 8:27 ` Jakub Jelinek 0 siblings, 1 reply; 43+ messages in thread From: Mark Wielaard @ 2016-02-23 7:55 UTC (permalink / raw) To: H.J. Lu Cc: Jeff Law, GCC Patches, Bernd Schmidt, Martin Sebor, Steve Ellcey, Jakub Jelinek, Manuel López-Ibáñez On Mon, 2016-02-22 at 19:20 -0800, H.J. Lu wrote: > It caused: > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69911 Apologies. Apparently main_input_filename can be NULL. I am not entirely sure when that happens. Or how I failed to see that test failure. I think I didn't have java enabled, causing libffi to be skipped. I am testing this patch that skips the test in that case: diff --git a/gcc/ChangeLog b/gcc/ChangeLog index 49f6c25..788f07e 100644 --- a/gcc/ChangeLog +++ b/gcc/ChangeLog @@ -1,3 +1,9 @@ +2016-02-23 Mark Wielaard <mjw@redhat.com> + + PR c/69911 + * cgraphunit.c (check_global_declaration): Check main_input_filename + is not NULL. + 2016-02-20 Mark Wielaard <mjw@redhat.com> PR c/28901 diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c index 27a073a..6c14be2 100644 --- a/gcc/cgraphunit.c +++ b/gcc/cgraphunit.c @@ -944,8 +944,9 @@ check_global_declaration (symtab_node *snode) || (((warn_unused_variable && ! TREE_READONLY (decl)) || (warn_unused_const_variable > 0 && TREE_READONLY (decl) && (warn_unused_const_variable == 2 - || filename_cmp (main_input_filename, - DECL_SOURCE_FILE (decl)) == 0))) + || (main_input_filename != NULL + && filename_cmp (main_input_filename, + DECL_SOURCE_FILE (decl)) == 0)))) && TREE_CODE (decl) == VAR_DECL)) && ! DECL_IN_SYSTEM_HEADER (decl) && ! snode->referred_to_p (/*include_self=*/false) ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] PR28901 Add two levels for -Wunused-const-variable. 2016-02-23 7:55 ` Mark Wielaard @ 2016-02-23 8:27 ` Jakub Jelinek 2016-02-23 8:54 ` Mark Wielaard 0 siblings, 1 reply; 43+ messages in thread From: Jakub Jelinek @ 2016-02-23 8:27 UTC (permalink / raw) To: Mark Wielaard Cc: H.J. Lu, Jeff Law, GCC Patches, Bernd Schmidt, Martin Sebor, Steve Ellcey, Manuel López-Ibáñez On Tue, Feb 23, 2016 at 08:55:40AM +0100, Mark Wielaard wrote: > On Mon, 2016-02-22 at 19:20 -0800, H.J. Lu wrote: > > It caused: > > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69911 > > Apologies. Apparently main_input_filename can be NULL. I am not entirely > sure when that happens. Or how I failed to see that test failure. I > think I didn't have java enabled, causing libffi to be skipped. > > I am testing this patch that skips the test in that case: Are you sure that is the problem? I think it doesn't hurt to check for non-NULL main_input_filename, perhaps some non-c-family languages might not set it, and this is in generic code, but at least on the gcc.target/i386/iamcu/test_passing_structs.c testcase and on one randomly selected libffi testcase I see the ICE from completely different reason - what is NULL is DECL_SOURCE_FILE (decl). decl is e.g. <var_decl 0x7ffff18b3cf0 *.LC3 type <record_type 0x7ffff18865e8 char7_struct sizes-gimplified type_0 BLK size <integer_cst 0x7ffff184cf30 constant 56> unit size <integer_cst 0x7ffff184cf00 constant 7> align 8 symtab 0 alias set 6 canonical type 0x7ffff18865e8 fields <field_decl 0x7ffff1889390 c1 type <integer_type 0x7ffff17175e8 char> QI file /usr/src/gcc/gcc/testsuite/gcc.target/i386/iamcu/test_passing_structs.c line 133 col 8 size <integer_cst 0x7ffff1713de0 constant 8> unit size <integer_cst 0x7ffff1713df8 constant 1> align 8 offset_align 32 offset <integer_cst 0x7ffff1713cd8 constant 0> bit offset <integer_cst 0x7ffff1713d38 constant 0> context <record_type 0x7ffff18865e8 char7_struct> chain <field_decl 0x7ffff1889428 c2>> context <translation_unit_decl 0x7ffff171f708 D.2134> chain <type_decl 0x7ffff18892f8 D.2095>> readonly addressable static ignored in-constant-pool BLK file (null) line 0 col 0 size <integer_cst 0x7ffff184cf30 56> unit size <integer_cst 0x7ffff184cf00 7> align 32 initial <constructor 0x7ffff188e5e8>> We are not really going to warn about this anyway, e.g. because it is DECL_ARTIFICIAL, but that is checked only in a much later condition. So, I think you should check also for NULL DECL_SOURCE_FILE (and treat it as possibly in a header). Unfortunately DECL_SOURCE_FILE involves a function call, so you might want to cache it in some automatic variable. && (warn_unused_const_variable == 2 || (main_input_filename != NULL && (decl_file = DECL_SOURCE_FILE (decl)) != NULL && filename_cmp (main_input_filename, decl_file) == 0)))) Jakub ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] PR28901 Add two levels for -Wunused-const-variable. 2016-02-23 8:27 ` Jakub Jelinek @ 2016-02-23 8:54 ` Mark Wielaard 2016-02-23 8:57 ` Jakub Jelinek 0 siblings, 1 reply; 43+ messages in thread From: Mark Wielaard @ 2016-02-23 8:54 UTC (permalink / raw) To: Jakub Jelinek Cc: H.J. Lu, Jeff Law, GCC Patches, Bernd Schmidt, Martin Sebor, Steve Ellcey, Manuel López-Ibáñez On Tue, 2016-02-23 at 09:26 +0100, Jakub Jelinek wrote: > On Tue, Feb 23, 2016 at 08:55:40AM +0100, Mark Wielaard wrote: > > On Mon, 2016-02-22 at 19:20 -0800, H.J. Lu wrote: > > > It caused: > > > > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69911 > > > > Apologies. Apparently main_input_filename can be NULL. I am not entirely > > sure when that happens. Or how I failed to see that test failure. I > > think I didn't have java enabled, causing libffi to be skipped. > > > > I am testing this patch that skips the test in that case: > > Are you sure that is the problem? No :) I was still bootstrapping. I did see it didn't solve the issue but hadn't understood why yet... > I think it doesn't hurt to check for non-NULL main_input_filename, perhaps > some non-c-family languages might not set it, and this is in generic code, > but at least on the gcc.target/i386/iamcu/test_passing_structs.c testcase and on one > randomly selected libffi testcase I see the ICE from completely different > reason - what is NULL is DECL_SOURCE_FILE (decl). Thanks, that makes more sense than my first hypothesis. > We are not really going to warn about this anyway, e.g. because > it is DECL_ARTIFICIAL, but that is checked only in a much later > condition. So, I think you should check also for NULL DECL_SOURCE_FILE > (and treat it as possibly in a header). Unfortunately > DECL_SOURCE_FILE involves a function call, so you might want to cache > it in some automatic variable. > && (warn_unused_const_variable == 2 > || (main_input_filename != NULL > && (decl_file = DECL_SOURCE_FILE (decl)) != NULL > && filename_cmp (main_input_filename, > decl_file) == 0)))) That looks right. Now testing: diff --git a/gcc/ChangeLog b/gcc/ChangeLog index 49f6c25..e9e1aab 100644 --- a/gcc/ChangeLog +++ b/gcc/ChangeLog @@ -1,3 +1,10 @@ +2016-02-23 Mark Wielaard <mjw@redhat.com> + Jakub Jelinek <jakub@redhat.com> + + PR c/69911 + * cgraphunit.c (check_global_declaration): Check main_input_filename + and DECL_SOURCE_FILE are not NULL. + 2016-02-20 Mark Wielaard <mjw@redhat.com> PR c/28901 diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c index 27a073a..8b3fddc 100644 --- a/gcc/cgraphunit.c +++ b/gcc/cgraphunit.c @@ -917,6 +917,7 @@ walk_polymorphic_call_targets (hash_set<void *> *reachable_call_targets, static void check_global_declaration (symtab_node *snode) { + const char *decl_file; tree decl = snode->decl; /* Warn about any function declared static but not defined. We don't @@ -944,8 +945,10 @@ check_global_declaration (symtab_node *snode) || (((warn_unused_variable && ! TREE_READONLY (decl)) || (warn_unused_const_variable > 0 && TREE_READONLY (decl) && (warn_unused_const_variable == 2 - || filename_cmp (main_input_filename, - DECL_SOURCE_FILE (decl)) == 0))) + || (main_input_filename != NULL + && (decl_file = DECL_SOURCE_FILE (decl)) != NULL + && filename_cmp (main_input_filename, + decl_file) == 0)))) && TREE_CODE (decl) == VAR_DECL)) && ! DECL_IN_SYSTEM_HEADER (decl) && ! snode->referred_to_p (/*include_self=*/false) ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] PR28901 Add two levels for -Wunused-const-variable. 2016-02-23 8:54 ` Mark Wielaard @ 2016-02-23 8:57 ` Jakub Jelinek 2016-02-23 9:51 ` Manuel López-Ibáñez 0 siblings, 1 reply; 43+ messages in thread From: Jakub Jelinek @ 2016-02-23 8:57 UTC (permalink / raw) To: Mark Wielaard Cc: H.J. Lu, Jeff Law, GCC Patches, Bernd Schmidt, Martin Sebor, Steve Ellcey, Manuel López-Ibáñez On Tue, Feb 23, 2016 at 09:53:57AM +0100, Mark Wielaard wrote: > --- a/gcc/ChangeLog > +++ b/gcc/ChangeLog > @@ -1,3 +1,10 @@ > +2016-02-23 Mark Wielaard <mjw@redhat.com> > + Jakub Jelinek <jakub@redhat.com> > + > + PR c/69911 > + * cgraphunit.c (check_global_declaration): Check main_input_filename > + and DECL_SOURCE_FILE are not NULL. > + > 2016-02-20 Mark Wielaard <mjw@redhat.com> This is ok for trunk if it passes testing. Thanks. > diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c > index 27a073a..8b3fddc 100644 > --- a/gcc/cgraphunit.c > +++ b/gcc/cgraphunit.c > @@ -917,6 +917,7 @@ walk_polymorphic_call_targets (hash_set<void *> *reachable_call_targets, > static void > check_global_declaration (symtab_node *snode) > { > + const char *decl_file; > tree decl = snode->decl; > > /* Warn about any function declared static but not defined. We don't > @@ -944,8 +945,10 @@ check_global_declaration (symtab_node *snode) > || (((warn_unused_variable && ! TREE_READONLY (decl)) > || (warn_unused_const_variable > 0 && TREE_READONLY (decl) > && (warn_unused_const_variable == 2 > - || filename_cmp (main_input_filename, > - DECL_SOURCE_FILE (decl)) == 0))) > + || (main_input_filename != NULL > + && (decl_file = DECL_SOURCE_FILE (decl)) != NULL > + && filename_cmp (main_input_filename, > + decl_file) == 0)))) > && TREE_CODE (decl) == VAR_DECL)) > && ! DECL_IN_SYSTEM_HEADER (decl) > && ! snode->referred_to_p (/*include_self=*/false) Jakub ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] PR28901 Add two levels for -Wunused-const-variable. 2016-02-23 8:57 ` Jakub Jelinek @ 2016-02-23 9:51 ` Manuel López-Ibáñez 2016-02-23 9:55 ` Jakub Jelinek 2016-02-23 12:10 ` Mark Wielaard 0 siblings, 2 replies; 43+ messages in thread From: Manuel López-Ibáñez @ 2016-02-23 9:51 UTC (permalink / raw) To: Jakub Jelinek, Mark Wielaard Cc: H.J. Lu, Jeff Law, GCC Patches, Bernd Schmidt, Martin Sebor, Steve Ellcey On 23/02/16 08:56, Jakub Jelinek wrote: > On Tue, Feb 23, 2016@09:53:57AM +0100, Mark Wielaard wrote: >> --- a/gcc/ChangeLog >> +++ b/gcc/ChangeLog >> @@ -1,3 +1,10 @@ >> +2016-02-23 Mark Wielaard <mjw@redhat.com> >> + Jakub Jelinek <jakub@redhat.com> >> + >> + PR c/69911 >> + * cgraphunit.c (check_global_declaration): Check main_input_filename >> + and DECL_SOURCE_FILE are not NULL. >> + >> 2016-02-20 Mark Wielaard <mjw@redhat.com> > > This is ok for trunk if it passes testing. Thanks. > >> diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c >> index 27a073a..8b3fddc 100644 >> --- a/gcc/cgraphunit.c >> +++ b/gcc/cgraphunit.c >> @@ -917,6 +917,7 @@ walk_polymorphic_call_targets (hash_set<void *> *reachable_call_targets, >> static void >> check_global_declaration (symtab_node *snode) >> { >> + const char *decl_file; >> tree decl = snode->decl; >> >> /* Warn about any function declared static but not defined. We don't >> @@ -944,8 +945,10 @@ check_global_declaration (symtab_node *snode) >> || (((warn_unused_variable && ! TREE_READONLY (decl)) >> || (warn_unused_const_variable > 0 && TREE_READONLY (decl) >> && (warn_unused_const_variable == 2 >> - || filename_cmp (main_input_filename, >> - DECL_SOURCE_FILE (decl)) == 0))) >> + || (main_input_filename != NULL >> + && (decl_file = DECL_SOURCE_FILE (decl)) != NULL >> + && filename_cmp (main_input_filename, >> + decl_file) == 0)))) Can we please please please hide this ugliness behind an (inline?) function such as bool in_main_file_at (location_t) in input.[ch]? The condition here is quickly becoming unreadable. Also because in the future somebody would want to re-implement this using MAIN_FILE_P() from line-map.h, which is faster. Cheers, Manuel. ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] PR28901 Add two levels for -Wunused-const-variable. 2016-02-23 9:51 ` Manuel López-Ibáñez @ 2016-02-23 9:55 ` Jakub Jelinek 2016-02-23 12:10 ` Mark Wielaard 1 sibling, 0 replies; 43+ messages in thread From: Jakub Jelinek @ 2016-02-23 9:55 UTC (permalink / raw) To: Manuel López-Ibáñez Cc: Mark Wielaard, H.J. Lu, Jeff Law, GCC Patches, Bernd Schmidt, Martin Sebor, Steve Ellcey On Tue, Feb 23, 2016 at 09:51:05AM +0000, Manuel López-Ibáñez wrote: > >>diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c > >>index 27a073a..8b3fddc 100644 > >>--- a/gcc/cgraphunit.c > >>+++ b/gcc/cgraphunit.c > >>@@ -917,6 +917,7 @@ walk_polymorphic_call_targets (hash_set<void *> *reachable_call_targets, > >> static void > >> check_global_declaration (symtab_node *snode) > >> { > >>+ const char *decl_file; > >> tree decl = snode->decl; > >> > >> /* Warn about any function declared static but not defined. We don't > >>@@ -944,8 +945,10 @@ check_global_declaration (symtab_node *snode) > >> || (((warn_unused_variable && ! TREE_READONLY (decl)) > >> || (warn_unused_const_variable > 0 && TREE_READONLY (decl) > >> && (warn_unused_const_variable == 2 > >>- || filename_cmp (main_input_filename, > >>- DECL_SOURCE_FILE (decl)) == 0))) > >>+ || (main_input_filename != NULL > >>+ && (decl_file = DECL_SOURCE_FILE (decl)) != NULL > >>+ && filename_cmp (main_input_filename, > >>+ decl_file) == 0)))) > > > Can we please please please hide this ugliness behind an (inline?) function > such as bool in_main_file_at (location_t) in input.[ch]? The condition here > is quickly becoming unreadable. > > Also because in the future somebody would want to re-implement this using > MAIN_FILE_P() from line-map.h, which is faster. I'm not against that, but I'd prefer if it is done incrementally, we don't want to keep the trunk broken too much for too long. So if Mark is already testing this patch, IMHO it is better to commit it in this shape sooner. Jakub ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] PR28901 Add two levels for -Wunused-const-variable. 2016-02-23 9:51 ` Manuel López-Ibáñez 2016-02-23 9:55 ` Jakub Jelinek @ 2016-02-23 12:10 ` Mark Wielaard 1 sibling, 0 replies; 43+ messages in thread From: Mark Wielaard @ 2016-02-23 12:10 UTC (permalink / raw) To: Manuel López-Ibáñez Cc: Jakub Jelinek, H.J. Lu, Jeff Law, GCC Patches, Bernd Schmidt, Martin Sebor, Steve Ellcey On Tue, 2016-02-23 at 09:51 +0000, Manuel López-Ibáñez wrote: > On 23/02/16 08:56, Jakub Jelinek wrote: > > On Tue, Feb 23, 2016@09:53:57AM +0100, Mark Wielaard wrote: > >> --- a/gcc/ChangeLog > >> +++ b/gcc/ChangeLog > >> @@ -1,3 +1,10 @@ > >> +2016-02-23 Mark Wielaard <mjw@redhat.com> > >> + Jakub Jelinek <jakub@redhat.com> > >> + > >> + PR c/69911 > >> + * cgraphunit.c (check_global_declaration): Check main_input_filename > >> + and DECL_SOURCE_FILE are not NULL. > >> + > >> 2016-02-20 Mark Wielaard <mjw@redhat.com> > > > > This is ok for trunk if it passes testing. Thanks. > > > >> diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c > >> index 27a073a..8b3fddc 100644 > >> --- a/gcc/cgraphunit.c > >> +++ b/gcc/cgraphunit.c > >> @@ -917,6 +917,7 @@ walk_polymorphic_call_targets (hash_set<void *> *reachable_call_targets, > >> static void > >> check_global_declaration (symtab_node *snode) > >> { > >> + const char *decl_file; > >> tree decl = snode->decl; > >> > >> /* Warn about any function declared static but not defined. We don't > >> @@ -944,8 +945,10 @@ check_global_declaration (symtab_node *snode) > >> || (((warn_unused_variable && ! TREE_READONLY (decl)) > >> || (warn_unused_const_variable > 0 && TREE_READONLY (decl) > >> && (warn_unused_const_variable == 2 > >> - || filename_cmp (main_input_filename, > >> - DECL_SOURCE_FILE (decl)) == 0))) > >> + || (main_input_filename != NULL > >> + && (decl_file = DECL_SOURCE_FILE (decl)) != NULL > >> + && filename_cmp (main_input_filename, > >> + decl_file) == 0)))) > > > Can we please please please hide this ugliness behind an (inline?) function > such as bool in_main_file_at (location_t) in input.[ch]? The condition here is > quickly becoming unreadable. > > Also because in the future somebody would want to re-implement this using > MAIN_FILE_P() from line-map.h, which is faster. Sorry, I pushed the minimum fix. I am too embarrassed already for breaking stuff to risk refactoring anything and making another dumb mistake. (My mistake was only checking the top-level compiler .sum files, I had forgotten to check the library ones). It might be a good idea to file a bug for this and write a patch to submit during next stage one if you believe this is a useful cleanup. Cheers, Mark ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables 2015-09-15 17:04 ` Steve Ellcey 2015-09-15 17:10 ` Jakub Jelinek @ 2015-09-15 17:20 ` Joseph Myers 1 sibling, 0 replies; 43+ messages in thread From: Joseph Myers @ 2015-09-15 17:20 UTC (permalink / raw) To: Steve Ellcey Cc: Bernd Schmidt, Mark Wielaard, Manuel López-Ibáñez, GCC Patches On Tue, 15 Sep 2015, Steve Ellcey wrote: > I am not sure I like this change. It broke the GLIBC build for me on > MIPS. Basically GLIBC has a header file with initialized static > constant globals (sysdeps/ieee754/dbl-64/atnat2.h contains tqpi1 and > qpi1) and that header file is included in multiple .c files like > sysdeps/ieee754/dbl-64/e_atan2.c that use some, but not all, of those > static constant variables. But between the various .c files all of the > globals are used somewhere, just not in every individual .c file. This > seems like a perfectly reasonable use of static globals and header files > that should not be identified as a warning. And glibc also uses -fmerge-all-constants, so this C++-style use of static variables should be just as efficient as using #define in all cases (though since the addresses of the variables aren't taken, it should be just as efficient even without -fmerge-all-constants). -- Joseph S. Myers joseph@codesourcery.com ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables 2015-09-13 12:51 ` Mark Wielaard 2015-09-13 13:36 ` Manuel López-Ibáñez @ 2015-09-17 12:41 ` Gerald Pfeifer 2015-09-17 16:27 ` Bernd Schmidt 1 sibling, 1 reply; 43+ messages in thread From: Gerald Pfeifer @ 2015-09-17 12:41 UTC (permalink / raw) To: Mark Wielaard; +Cc: Bernd Schmidt, gcc-patches On Sun, 13 Sep 2015, Mark Wielaard wrote: > Slightly adjusted patch attached. Now it is explicit that the warning is > enabled by -Wunused-variable for C, but not C++. There are testcases for > both C and C++ to check the defaults. And the hardcoded override is > removed for C++, so the user could enable it if they want. I believe making -Wunused-const-variable part of -Wall is not a good idea. For example, I have a nightly build of Wine with a nightly build of GCC. And my notifaction mail went from twently lines of warning to 6500 -- all coming from header files. Gerald ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables 2015-09-17 12:41 ` Gerald Pfeifer @ 2015-09-17 16:27 ` Bernd Schmidt 2015-09-17 16:32 ` Mark Wielaard 0 siblings, 1 reply; 43+ messages in thread From: Bernd Schmidt @ 2015-09-17 16:27 UTC (permalink / raw) To: Gerald Pfeifer, Mark Wielaard; +Cc: gcc-patches On 09/17/2015 02:01 PM, Gerald Pfeifer wrote: > On Sun, 13 Sep 2015, Mark Wielaard wrote: >> Slightly adjusted patch attached. Now it is explicit that the warning is >> enabled by -Wunused-variable for C, but not C++. There are testcases for >> both C and C++ to check the defaults. And the hardcoded override is >> removed for C++, so the user could enable it if they want. > > I believe making -Wunused-const-variable part of -Wall is not > a good idea. > > For example, I have a nightly build of Wine with a nightly build > of GCC. And my notifaction mail went from twently lines of warning > to 6500 -- all coming from header files. What does it warn about, do the warnings look valid? Bernd ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables 2015-09-17 16:27 ` Bernd Schmidt @ 2015-09-17 16:32 ` Mark Wielaard 2015-10-23 23:41 ` Gerald Pfeifer 0 siblings, 1 reply; 43+ messages in thread From: Mark Wielaard @ 2015-09-17 16:32 UTC (permalink / raw) To: Bernd Schmidt; +Cc: Gerald Pfeifer, gcc-patches On Thu, 2015-09-17 at 18:12 +0200, Bernd Schmidt wrote: > On 09/17/2015 02:01 PM, Gerald Pfeifer wrote: > > On Sun, 13 Sep 2015, Mark Wielaard wrote: > >> Slightly adjusted patch attached. Now it is explicit that the warning is > >> enabled by -Wunused-variable for C, but not C++. There are testcases for > >> both C and C++ to check the defaults. And the hardcoded override is > >> removed for C++, so the user could enable it if they want. > > > > I believe making -Wunused-const-variable part of -Wall is not > > a good idea. > > > > For example, I have a nightly build of Wine with a nightly build > > of GCC. And my notifaction mail went from twently lines of warning > > to 6500 -- all coming from header files. > > What does it warn about, do the warnings look valid? I was just taking a look. They come from constructs like: static const WCHAR INSTALLPROPERTY_VERSIONSTRINGW[] = {'V','e','r','s','i','o','n','S','t','r','i','n','g',0}; Using the more idiomatic and a bit more readable: #define INSTALLPROPERTY_VERSIONSTRINGW u"VersionString" gets rid of most of the issues. Cheers, Mark ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables 2015-09-17 16:32 ` Mark Wielaard @ 2015-10-23 23:41 ` Gerald Pfeifer 0 siblings, 0 replies; 43+ messages in thread From: Gerald Pfeifer @ 2015-10-23 23:41 UTC (permalink / raw) To: Mark Wielaard; +Cc: Bernd Schmidt, gcc-patches On Thu, 17 Sep 2015, Mark Wielaard wrote: >>> I believe making -Wunused-const-variable part of -Wall is not >>> a good idea. >>> >>> For example, I have a nightly build of Wine with a nightly build >>> of GCC. And my notifaction mail went from twently lines of warning >>> to 6500 -- all coming from header files. > I was just taking a look. They come from constructs like: > > static const WCHAR INSTALLPROPERTY_VERSIONSTRINGW[] = > {'V','e','r','s','i','o','n','S','t','r','i','n','g',0}; > > Using the more idiomatic and a bit more readable: > > #define INSTALLPROPERTY_VERSIONSTRINGW u"VersionString" > > gets rid of most of the issues. I am working to get Wine converted to at least the u"VersionString" flavor, and then ideally to use #define or to disable this new warning globally. Should there remain any other cases, I'll report them here. (Thanks for the hints, Mark!) Gerald ^ permalink raw reply [flat|nested] 43+ messages in thread
end of thread, other threads:[~2016-02-23 12:10 UTC | newest] Thread overview: 43+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-09-11 22:29 [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables Mark Wielaard 2015-09-11 22:40 ` Bernd Schmidt 2015-09-13 5:44 ` Mark Wielaard 2015-09-13 12:51 ` Mark Wielaard 2015-09-13 13:36 ` Manuel López-Ibáñez 2015-09-13 18:50 ` Mark Wielaard 2015-09-14 7:54 ` Bernd Schmidt 2015-09-15 17:04 ` Steve Ellcey 2015-09-15 17:10 ` Jakub Jelinek 2015-09-15 17:26 ` Steve Ellcey 2015-09-15 18:55 ` Mark Wielaard 2015-09-15 19:21 ` Mark Wielaard 2015-09-15 19:58 ` Joseph Myers 2015-09-19 2:57 ` Martin Sebor 2015-09-21 17:10 ` Steve Ellcey 2015-09-23 18:26 ` Jeff Law 2015-09-24 12:54 ` Mark Wielaard 2015-09-24 13:06 ` Bernd Schmidt 2015-09-24 16:38 ` Steve Ellcey 2015-09-24 18:40 ` Bernd Schmidt 2015-09-25 8:00 ` Trevor Saunders 2015-10-06 22:44 ` Steve Ellcey 2015-10-07 12:00 ` Bernd Schmidt 2015-10-24 15:00 ` Gerald Pfeifer 2015-10-24 15:11 ` Mark Wielaard 2016-02-21 1:43 ` [PATCH] PR28901 Add two levels for -Wunused-const-variable Mark Wielaard 2016-02-22 18:58 ` Jeff Law 2016-02-22 19:00 ` Jakub Jelinek 2016-02-22 19:02 ` Jeff Law 2016-02-22 22:43 ` Mark Wielaard 2016-02-23 3:21 ` H.J. Lu 2016-02-23 7:55 ` Mark Wielaard 2016-02-23 8:27 ` Jakub Jelinek 2016-02-23 8:54 ` Mark Wielaard 2016-02-23 8:57 ` Jakub Jelinek 2016-02-23 9:51 ` Manuel López-Ibáñez 2016-02-23 9:55 ` Jakub Jelinek 2016-02-23 12:10 ` Mark Wielaard 2015-09-15 17:20 ` [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables Joseph Myers 2015-09-17 12:41 ` Gerald Pfeifer 2015-09-17 16:27 ` Bernd Schmidt 2015-09-17 16:32 ` Mark Wielaard 2015-10-23 23:41 ` Gerald Pfeifer
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).