c++: Fix spurious fallthrough warning on break

Message ID 20180219162900.31733-1-siddhesh@sourceware.org
State New
Headers show
Series
  • c++: Fix spurious fallthrough warning on break
Related show

Commit Message

Siddhesh Poyarekar Feb. 19, 2018, 4:29 p.m.
The C++ frontend generates a break that results in the fallthrough
warning misfiring in nested switch blocks where cases in the inner
switch block return, rendering the break pointless.  The fallthrough
detection in finish_break_stmt does not work either because the
condition is encoded as an IF_STMT and not a COND_EXPR.

Fix this by adding a condition for IF_STMT in the
langhooks.block_may_fallthru for C++.  Fix tested on x86_64.  Full
testsuite run is in progress.

Siddhesh

gcc/cp
	* cp-objcp-common.c (cxx_block_may_fallthru): Add case for
	IF_STMT.

gcc/testsuite
	* g++.dg/nested-switch.C: New test case.
---
 gcc/cp/cp-objcp-common.c             |  5 +++++
 gcc/testsuite/g++.dg/nested-switch.C | 30 ++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+)
 create mode 100644 gcc/testsuite/g++.dg/nested-switch.C

-- 
2.14.3

Comments

Siddhesh Poyarekar Feb. 20, 2018, 9:23 a.m. | #1
On Monday 19 February 2018 09:59 PM, Siddhesh Poyarekar wrote:
> The C++ frontend generates a break that results in the fallthrough

> warning misfiring in nested switch blocks where cases in the inner

> switch block return, rendering the break pointless.  The fallthrough

> detection in finish_break_stmt does not work either because the

> condition is encoded as an IF_STMT and not a COND_EXPR.

> 

> Fix this by adding a condition for IF_STMT in the

> langhooks.block_may_fallthru for C++.  Fix tested on x86_64.  Full

> testsuite run is in progress.


Now confirmed that the patch does not introduce any new regressions.

Siddhesh
Jakub Jelinek Feb. 20, 2018, 9:44 a.m. | #2
On Mon, Feb 19, 2018 at 09:59:00PM +0530, Siddhesh Poyarekar wrote:
> The C++ frontend generates a break that results in the fallthrough

> warning misfiring in nested switch blocks where cases in the inner

> switch block return, rendering the break pointless.  The fallthrough

> detection in finish_break_stmt does not work either because the

> condition is encoded as an IF_STMT and not a COND_EXPR.

> 

> Fix this by adding a condition for IF_STMT in the

> langhooks.block_may_fallthru for C++.  Fix tested on x86_64.  Full

> testsuite run is in progress.

> 

> Siddhesh

> 

> gcc/cp

> 	* cp-objcp-common.c (cxx_block_may_fallthru): Add case for

> 	IF_STMT.

> 

> gcc/testsuite

> 	* g++.dg/nested-switch.C: New test case.


C++ testcase shouldn't be put directly into g++.dg/ (though yes, several
have been), but into a subdirectory.  In this case, because it is a (bogus)
warning, it should best go into g++.dg/warn/, and perhaps best named as
g++.dg/warn/Wimplicit-fallthrough-3.C .

> --- /dev/null

> +++ b/gcc/testsuite/g++.dg/nested-switch.C

> @@ -0,0 +1,30 @@

> +// Verify that there are no spurious warnings in nested switch statements due

> +// to the unnecessary break in the inner switch block.

> +// { dg-do compile }

> +// { dg-options "-Werror=implicit-fallthrough" } */


I'd go just for -Wimplicit-fallthrough.

> +

> +int

> +foo (int c1, int c2, int c3)

> +{

> +  switch (c2)

> +    {

> +    case 0:   

> +      switch (c3) {


And turn the above line into
      switch (c3) {	// { dg-bogus "may fall through" }

> +	case 0:

> +	  if (c1)

> +	    return 1;

> +	  else

> +	    return 2;

> +	  break;

> +

> +	default:

> +	  return 3;

> +      }

> +

> +    case 1: 

> +      return 4;

> +    default:

> +      return 5;

> +      break;

> +    }

> +}


Ok for trunk with those nits, thanks.

	Jakub
Siddhesh Poyarekar Feb. 20, 2018, 11:13 a.m. | #3
On Tuesday 20 February 2018 03:14 PM, Jakub Jelinek wrote:
> On Mon, Feb 19, 2018 at 09:59:00PM +0530, Siddhesh Poyarekar wrote:

>> The C++ frontend generates a break that results in the fallthrough

>> warning misfiring in nested switch blocks where cases in the inner

>> switch block return, rendering the break pointless.  The fallthrough

>> detection in finish_break_stmt does not work either because the

>> condition is encoded as an IF_STMT and not a COND_EXPR.

>>

>> Fix this by adding a condition for IF_STMT in the

>> langhooks.block_may_fallthru for C++.  Fix tested on x86_64.  Full

>> testsuite run is in progress.

>>

>> Siddhesh

>>

>> gcc/cp

>> 	* cp-objcp-common.c (cxx_block_may_fallthru): Add case for

>> 	IF_STMT.

>>

>> gcc/testsuite

>> 	* g++.dg/nested-switch.C: New test case.

> 

> C++ testcase shouldn't be put directly into g++.dg/ (though yes, several

> have been), but into a subdirectory.  In this case, because it is a (bogus)

> warning, it should best go into g++.dg/warn/, and perhaps best named as

> g++.dg/warn/Wimplicit-fallthrough-3.C .

> 

>> --- /dev/null

>> +++ b/gcc/testsuite/g++.dg/nested-switch.C

>> @@ -0,0 +1,30 @@

>> +// Verify that there are no spurious warnings in nested switch statements due

>> +// to the unnecessary break in the inner switch block.

>> +// { dg-do compile }

>> +// { dg-options "-Werror=implicit-fallthrough" } */

> 

> I'd go just for -Wimplicit-fallthrough.

> 

>> +

>> +int

>> +foo (int c1, int c2, int c3)

>> +{

>> +  switch (c2)

>> +    {

>> +    case 0:   

>> +      switch (c3) {

> 

> And turn the above line into

>       switch (c3) {	// { dg-bogus "may fall through" }

> 

>> +	case 0:

>> +	  if (c1)

>> +	    return 1;

>> +	  else

>> +	    return 2;

>> +	  break;

>> +

>> +	default:

>> +	  return 3;

>> +      }

>> +

>> +    case 1: 

>> +      return 4;

>> +    default:

>> +      return 5;

>> +      break;

>> +    }

>> +}

> 

> Ok for trunk with those nits, thanks.


Thanks, fixed up and pushed.

Siddhesh

Patch

diff --git a/gcc/cp/cp-objcp-common.c b/gcc/cp/cp-objcp-common.c
index a45dda4d012..5289a89e486 100644
--- a/gcc/cp/cp-objcp-common.c
+++ b/gcc/cp/cp-objcp-common.c
@@ -349,6 +349,11 @@  cxx_block_may_fallthru (const_tree stmt)
     case THROW_EXPR:
       return false;
 
+    case IF_STMT:
+      if (block_may_fallthru (THEN_CLAUSE (stmt)))
+	return true;
+      return block_may_fallthru (ELSE_CLAUSE (stmt));
+
     case SWITCH_STMT:
       return (!SWITCH_STMT_ALL_CASES_P (stmt)
 	      || !SWITCH_STMT_NO_BREAK_P (stmt)
diff --git a/gcc/testsuite/g++.dg/nested-switch.C b/gcc/testsuite/g++.dg/nested-switch.C
new file mode 100644
index 00000000000..46412dd293f
--- /dev/null
+++ b/gcc/testsuite/g++.dg/nested-switch.C
@@ -0,0 +1,30 @@ 
+// Verify that there are no spurious warnings in nested switch statements due
+// to the unnecessary break in the inner switch block.
+// { dg-do compile }
+// { dg-options "-Werror=implicit-fallthrough" } */
+
+int
+foo (int c1, int c2, int c3)
+{
+  switch (c2)
+    {
+    case 0:   
+      switch (c3) {
+	case 0:
+	  if (c1)
+	    return 1;
+	  else
+	    return 2;
+	  break;
+
+	default:
+	  return 3;
+      }
+
+    case 1: 
+      return 4;
+    default:
+      return 5;
+      break;
+    }
+}