diff mbox series

[v3] c, c++: -Wswitch warning on [[maybe_unused]] enumerator [PR105497]

Message ID YoQ1+Y275lKujuab@redhat.com
State New
Headers show
Series [v3] c, c++: -Wswitch warning on [[maybe_unused]] enumerator [PR105497] | expand

Commit Message

Marek Polacek May 17, 2022, 11:55 p.m. UTC 
On Tue, May 10, 2022 at 09:54:12AM -0400, Marek Polacek wrote:
> On Tue, May 10, 2022 at 08:58:46AM -0400, Jason Merrill wrote:
> > On 5/7/22 18:26, Marek Polacek wrote:
> > > Corrected version that avoids an uninitialized warning:
> > > 
> > > This PR complains that we emit the "enumeration value not handled in
> > > switch" warning even though the enumerator was marked with the
> > > [[maybe_unused]] attribute.
> > > 
> > > The first snag was that I couldn't just check TREE_USED, because
> > > the enumerator could have been used earlier in the function, which
> > > doesn't play well with the c_do_switch_warnings warning.  Instead,
> > > I had to check the attributes on the CONST_DECL directly, which led
> > > to the second, and worse, snag: in C we don't have direct access to
> > > the CONST_DECL for the enumerator.
> > 
> > I wonder if you want to change that instead of working around it?
> 
> I wouldn't mind looking into that; I've hit this discrepancy numerous
> times throughout the years and it'd be good to unify it so that the
> c-common code doesn't need to hack around it.
>  
> Let's see how far I'll get...
 
Now done (r13-575), which makes this patch a piece of cake.

Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?

-- >8 --
This PR complains that we emit the "enumeration value not handled in
switch" warning even though the enumerator was marked with the
[[maybe_unused]] attribute.

I couldn't just check TREE_USED, because the enumerator could have been
used earlier in the function, which doesn't play well with the
c_do_switch_warnings warning.  Instead, I had to check the attributes on
the CONST_DECL.  This is easy since the TYPE_VALUES of an enum type are
now consistent between C and C++, both of which store the CONST_DECL in
its TREE_VALUE.

	PR c++/105497

gcc/c-family/ChangeLog:

	* c-warn.cc (c_do_switch_warnings): Don't warn about unhandled
	enumerator when it was marked with attribute unused.

gcc/testsuite/ChangeLog:

	* c-c++-common/Wswitch-1.c: New test.
	* g++.dg/warn/Wswitch-4.C: New test.
---
 gcc/c-family/c-warn.cc                 | 11 +++++-
 gcc/testsuite/c-c++-common/Wswitch-1.c | 29 ++++++++++++++
 gcc/testsuite/g++.dg/warn/Wswitch-4.C  | 52 ++++++++++++++++++++++++++
 3 files changed, 90 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/c-c++-common/Wswitch-1.c
 create mode 100644 gcc/testsuite/g++.dg/warn/Wswitch-4.C


base-commit: 1bfb823e2a7346ef55bd53a5354770599f7a550b

Comments

Jason Merrill May 18, 2022, 2:24 p.m. UTC | #1 
On 5/17/22 19:55, Marek Polacek wrote:
> On Tue, May 10, 2022 at 09:54:12AM -0400, Marek Polacek wrote:
>> On Tue, May 10, 2022 at 08:58:46AM -0400, Jason Merrill wrote:
>>> On 5/7/22 18:26, Marek Polacek wrote:
>>>> Corrected version that avoids an uninitialized warning:
>>>>
>>>> This PR complains that we emit the "enumeration value not handled in
>>>> switch" warning even though the enumerator was marked with the
>>>> [[maybe_unused]] attribute.
>>>>
>>>> The first snag was that I couldn't just check TREE_USED, because
>>>> the enumerator could have been used earlier in the function, which
>>>> doesn't play well with the c_do_switch_warnings warning.  Instead,
>>>> I had to check the attributes on the CONST_DECL directly, which led
>>>> to the second, and worse, snag: in C we don't have direct access to
>>>> the CONST_DECL for the enumerator.
>>>
>>> I wonder if you want to change that instead of working around it?
>>
>> I wouldn't mind looking into that; I've hit this discrepancy numerous
>> times throughout the years and it'd be good to unify it so that the
>> c-common code doesn't need to hack around it.
>>   
>> Let's see how far I'll get...
>   
> Now done (r13-575), which makes this patch a piece of cake.
> 
> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?

OK.

> -- >8 --
> This PR complains that we emit the "enumeration value not handled in
> switch" warning even though the enumerator was marked with the
> [[maybe_unused]] attribute.
> 
> I couldn't just check TREE_USED, because the enumerator could have been
> used earlier in the function, which doesn't play well with the
> c_do_switch_warnings warning.  Instead, I had to check the attributes on
> the CONST_DECL.  This is easy since the TYPE_VALUES of an enum type are
> now consistent between C and C++, both of which store the CONST_DECL in
> its TREE_VALUE.
> 
> 	PR c++/105497
> 
> gcc/c-family/ChangeLog:
> 
> 	* c-warn.cc (c_do_switch_warnings): Don't warn about unhandled
> 	enumerator when it was marked with attribute unused.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* c-c++-common/Wswitch-1.c: New test.
> 	* g++.dg/warn/Wswitch-4.C: New test.
> ---
>   gcc/c-family/c-warn.cc                 | 11 +++++-
>   gcc/testsuite/c-c++-common/Wswitch-1.c | 29 ++++++++++++++
>   gcc/testsuite/g++.dg/warn/Wswitch-4.C  | 52 ++++++++++++++++++++++++++
>   3 files changed, 90 insertions(+), 2 deletions(-)
>   create mode 100644 gcc/testsuite/c-c++-common/Wswitch-1.c
>   create mode 100644 gcc/testsuite/g++.dg/warn/Wswitch-4.C
> 
> diff --git a/gcc/c-family/c-warn.cc b/gcc/c-family/c-warn.cc
> index cae89294aea..ea7335f3edf 100644
> --- a/gcc/c-family/c-warn.cc
> +++ b/gcc/c-family/c-warn.cc
> @@ -1738,8 +1738,8 @@ c_do_switch_warnings (splay_tree cases, location_t switch_location,
>     for (chain = TYPE_VALUES (type); chain; chain = TREE_CHAIN (chain))
>       {
>         tree value = TREE_VALUE (chain);
> -      if (TREE_CODE (value) == CONST_DECL)
> -	value = DECL_INITIAL (value);
> +      tree attrs = DECL_ATTRIBUTES (value);
> +      value = DECL_INITIAL (value);
>         node = splay_tree_lookup (cases, (splay_tree_key) value);
>         if (node)
>   	{
> @@ -1769,6 +1769,13 @@ c_do_switch_warnings (splay_tree cases, location_t switch_location,
>         /* We've now determined that this enumerated literal isn't
>   	 handled by the case labels of the switch statement.  */
>   
> +      /* Don't warn if the enumerator was marked as unused.  We can't use
> +	 TREE_USED here: it could have been set on the enumerator if the
> +	 enumerator was used earlier.  */
> +      if (lookup_attribute ("unused", attrs)
> +	  || lookup_attribute ("maybe_unused", attrs))
> +	continue;
> +
>         /* If the switch expression is a constant, we only really care
>   	 about whether that constant is handled by the switch.  */
>         if (cond && tree_int_cst_compare (cond, value))
> diff --git a/gcc/testsuite/c-c++-common/Wswitch-1.c b/gcc/testsuite/c-c++-common/Wswitch-1.c
> new file mode 100644
> index 00000000000..de9ee03b0a3
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/Wswitch-1.c
> @@ -0,0 +1,29 @@
> +/* PR c++/105497 */
> +/* { dg-options "-Wswitch" } */
> +
> +enum E {
> +  A,
> +  B,
> +  C __attribute((unused)),
> +  D
> +};
> +
> +void
> +g (enum E e)
> +{
> +  switch (e)
> +    {
> +    case A:
> +    case B:
> +    case D:
> +      break;
> +    }
> +
> +  switch (e) // { dg-warning "not handled in switch" }
> +    {
> +    case A:
> +    case B:
> +    case C:
> +      break;
> +    }
> +}
> diff --git a/gcc/testsuite/g++.dg/warn/Wswitch-4.C b/gcc/testsuite/g++.dg/warn/Wswitch-4.C
> new file mode 100644
> index 00000000000..553a57d777b
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/warn/Wswitch-4.C
> @@ -0,0 +1,52 @@
> +// PR c++/105497
> +// { dg-do compile { target c++11 } }
> +// { dg-options "-Wswitch" }
> +
> +enum class Button
> +{
> +    Left,
> +    Right,
> +    Middle,
> +    NumberOfButtons [[maybe_unused]]
> +};
> +
> +enum class Sound
> +{
> +  Bark,
> +  Meow,
> +  Hiss,
> +  Moo __attribute((unused))
> +};
> +
> +enum class Chordata
> +{
> +  Urochordata,
> +  Cephalochordata,
> +  Vertebrata
> +};
> +
> +int main()
> +{
> +  Button b = Button::Left;
> +  switch (b) { // { dg-bogus "not handled" }
> +        case Button::Left:
> +        case Button::Right:
> +        case Button::Middle:
> +            break;
> +    }
> +
> +  Sound s = Sound::Bark;
> +  switch (s) { // { dg-bogus "not handled" }
> +    case Sound::Bark:
> +    case Sound::Meow:
> +    case Sound::Hiss:
> +      break;
> +  }
> +
> +  Chordata c = Chordata::Vertebrata;
> +  switch (c) { // { dg-warning "not handled" }
> +    case Chordata::Cephalochordata:
> +    case Chordata::Vertebrata:
> +      break;
> +  }
> +}
> 
> base-commit: 1bfb823e2a7346ef55bd53a5354770599f7a550b
diff mbox series

Patch

diff --git a/gcc/c-family/c-warn.cc b/gcc/c-family/c-warn.cc
index cae89294aea..ea7335f3edf 100644
--- a/gcc/c-family/c-warn.cc
+++ b/gcc/c-family/c-warn.cc
@@ -1738,8 +1738,8 @@  c_do_switch_warnings (splay_tree cases, location_t switch_location,
   for (chain = TYPE_VALUES (type); chain; chain = TREE_CHAIN (chain))
     {
       tree value = TREE_VALUE (chain);
-      if (TREE_CODE (value) == CONST_DECL)
-	value = DECL_INITIAL (value);
+      tree attrs = DECL_ATTRIBUTES (value);
+      value = DECL_INITIAL (value);
       node = splay_tree_lookup (cases, (splay_tree_key) value);
       if (node)
 	{
@@ -1769,6 +1769,13 @@  c_do_switch_warnings (splay_tree cases, location_t switch_location,
       /* We've now determined that this enumerated literal isn't
 	 handled by the case labels of the switch statement.  */
 
+      /* Don't warn if the enumerator was marked as unused.  We can't use
+	 TREE_USED here: it could have been set on the enumerator if the
+	 enumerator was used earlier.  */
+      if (lookup_attribute ("unused", attrs)
+	  || lookup_attribute ("maybe_unused", attrs))
+	continue;
+
       /* If the switch expression is a constant, we only really care
 	 about whether that constant is handled by the switch.  */
       if (cond && tree_int_cst_compare (cond, value))
diff --git a/gcc/testsuite/c-c++-common/Wswitch-1.c b/gcc/testsuite/c-c++-common/Wswitch-1.c
new file mode 100644
index 00000000000..de9ee03b0a3
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/Wswitch-1.c
@@ -0,0 +1,29 @@ 
+/* PR c++/105497 */
+/* { dg-options "-Wswitch" } */
+
+enum E {
+  A,
+  B,
+  C __attribute((unused)),
+  D
+};
+
+void
+g (enum E e)
+{
+  switch (e)
+    {
+    case A:
+    case B:
+    case D:
+      break;
+    }
+
+  switch (e) // { dg-warning "not handled in switch" }
+    {
+    case A:
+    case B:
+    case C:
+      break;
+    }
+}
diff --git a/gcc/testsuite/g++.dg/warn/Wswitch-4.C b/gcc/testsuite/g++.dg/warn/Wswitch-4.C
new file mode 100644
index 00000000000..553a57d777b
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wswitch-4.C
@@ -0,0 +1,52 @@ 
+// PR c++/105497
+// { dg-do compile { target c++11 } }
+// { dg-options "-Wswitch" }
+
+enum class Button
+{
+    Left,
+    Right,
+    Middle,
+    NumberOfButtons [[maybe_unused]]
+};
+
+enum class Sound
+{
+  Bark,
+  Meow,
+  Hiss,
+  Moo __attribute((unused))
+};
+
+enum class Chordata
+{
+  Urochordata,
+  Cephalochordata,
+  Vertebrata
+};
+
+int main()
+{
+  Button b = Button::Left;
+  switch (b) { // { dg-bogus "not handled" }
+        case Button::Left:
+        case Button::Right:
+        case Button::Middle:
+            break;
+    }
+
+  Sound s = Sound::Bark;
+  switch (s) { // { dg-bogus "not handled" }
+    case Sound::Bark:
+    case Sound::Meow:
+    case Sound::Hiss:
+      break;
+  }
+
+  Chordata c = Chordata::Vertebrata;
+  switch (c) { // { dg-warning "not handled" }
+    case Chordata::Cephalochordata:
+    case Chordata::Vertebrata:
+      break;
+  }
+}