[PATCH] PR28901 -Wunused-variable ignores unused const initialised variables

[PATCH] PR28901 -Wunused-variable ignores unused const initialised variables

[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);
 }
 

Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables

[Bernd Schmidt]

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

Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables

[Mark Wielaard]

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

Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables

[Mark Wielaard]

[-- 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);
 }
 

Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables

[Manuel López-Ibáñez]

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.

Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables

[Mark Wielaard]

[-- 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);
 }
 

Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables

[Bernd Schmidt]

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

Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables

[Steve Ellcey]

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

Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables

[Jakub Jelinek]

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

Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables

[Joseph Myers]

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

Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables

[Steve Ellcey]

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

Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables

[Mark Wielaard]

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

Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables

[Mark Wielaard]

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))
        {

Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables

[Joseph Myers]

timezone/private.h comes verbatim from the tzcode project and should not 
be modified locally in glibc.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables

[Gerald Pfeifer]

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

Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables

[Bernd Schmidt]

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

Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables

[Mark Wielaard]

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

Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables

[Martin Sebor]

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

Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables

[Steve Ellcey]

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

Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables

[Jeff Law]

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

Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables

[Mark Wielaard]

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

Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables

[Bernd Schmidt]

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

Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables

[Steve Ellcey]

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

Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables

[Bernd Schmidt]

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

Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables

[Trevor Saunders]

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

Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables

[Steve Ellcey]

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

Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables

[Bernd Schmidt]

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

Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables

[Gerald Pfeifer]

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

Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables

[Gerald Pfeifer]

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

Re: [PATCH] PR28901 -Wunused-variable ignores unused const initialised variables

[Mark Wielaard]

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

[PATCH] PR28901 Add two levels for -Wunused-const-variable.

[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

Re: [PATCH] PR28901 Add two levels for -Wunused-const-variable.

[Jeff Law]

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

Re: [PATCH] PR28901 Add two levels for -Wunused-const-variable.

[Jakub Jelinek]

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

Re: [PATCH] PR28901 Add two levels for -Wunused-const-variable.

[Jeff Law]

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

Re: [PATCH] PR28901 Add two levels for -Wunused-const-variable.

[Mark Wielaard]

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

Re: [PATCH] PR28901 Add two levels for -Wunused-const-variable.

[H.J. Lu]

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.

Re: [PATCH] PR28901 Add two levels for -Wunused-const-variable.

[Mark Wielaard]

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)

Re: [PATCH] PR28901 Add two levels for -Wunused-const-variable.

[Jakub Jelinek]

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

Re: [PATCH] PR28901 Add two levels for -Wunused-const-variable.

[Mark Wielaard]

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)

Re: [PATCH] PR28901 Add two levels for -Wunused-const-variable.

[Jakub Jelinek]

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

Re: [PATCH] PR28901 Add two levels for -Wunused-const-variable.

[Manuel López-Ibáñez]

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.

Re: [PATCH] PR28901 Add two levels for -Wunused-const-variable.

[Jakub Jelinek]

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

Re: [PATCH] PR28901 Add two levels for -Wunused-const-variable.

[Mark Wielaard]

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