Re: [C++ Patch/RFC] PR 67980 ("left shift count is negative [-Wshift-count-negative] generated for unreachable code")

[Jason Merrill]

On Tue, Oct 18, 2016 at 4:26 PM, Paolo Carlini  wrote:
> Thus, I'm back to one of my first tries earlier today: a much more
> conservative change which uses fold_non_dependent_expr only for the purpose
> of suppressing the unwanted warnings, see the below.

This looks right; in general we use early folding only to control warnings.  OK.

Jason

Re: [C++ Patch/RFC] PR 67980 ("left shift count is negative [-Wshift-count-negative] generated for unreachable code")

[Paolo Carlini]

.. I'm still looking for some directions about the best way to handle 
this issue: anyway, in case it wasn't clear, the second patch I posted 
passes testing.


Thanks,
Paolo.

Re: [C++ Patch/RFC] PR 67980 ("left shift count is negative [-Wshift-count-negative] generated for unreachable code")

[Paolo Carlini]

... sorry, what I sent earlier in fact causes a regression in the 
libstdc++-v3 testsuite: 23_containers/list/61347.cc.


Thus, I'm back to one of my first tries earlier today: a much more 
conservative change which uses fold_non_dependent_expr only for the 
purpose of suppressing the unwanted warnings, see the below.


Thanks,
Paolo.

///
Index: cp/pt.c
===
--- cp/pt.c (revision 241313)
+++ cp/pt.c (working copy)
@@ -15410,7 +15410,14 @@
   if (IF_STMT_CONSTEXPR_P (t) && integer_zerop (tmp))
/* Don't instantiate the THEN_CLAUSE. */;
   else
-   RECUR (THEN_CLAUSE (t));
+   {
+ bool inhibit = integer_zerop (fold_non_dependent_expr (tmp));
+ if (inhibit)
+   ++c_inhibit_evaluation_warnings;
+ RECUR (THEN_CLAUSE (t));
+ if (inhibit)
+   --c_inhibit_evaluation_warnings;
+   }
   finish_then_clause (stmt);
 
   if (IF_STMT_CONSTEXPR_P (t) && integer_nonzerop (tmp))
@@ -15417,8 +15424,13 @@
/* Don't instantiate the ELSE_CLAUSE. */;
   else if (ELSE_CLAUSE (t))
{
+ bool inhibit = integer_nonzerop (fold_non_dependent_expr (tmp));
  begin_else_clause (stmt);
+ if (inhibit)
+   ++c_inhibit_evaluation_warnings;
  RECUR (ELSE_CLAUSE (t));
+ if (inhibit)
+   --c_inhibit_evaluation_warnings;
  finish_else_clause (stmt);
}
 
Index: testsuite/g++.dg/cpp1y/pr67980.C
===
--- testsuite/g++.dg/cpp1y/pr67980.C(revision 0)
+++ testsuite/g++.dg/cpp1y/pr67980.C(working copy)
@@ -0,0 +1,23 @@
+// { dg-do compile { target c++14 } }
+
+template 
+constexpr T cpp14_constexpr_then(T value) {
+  if (Y < 0)
+return (value << -Y);
+  else
+return 0;
+}
+
+template 
+constexpr T cpp14_constexpr_else(T value) {
+  if (Y > 0)
+return 0;
+  else
+return (value << -Y);
+}
+
+int main()
+{
+  cpp14_constexpr_then<1>(0);
+  cpp14_constexpr_else<1>(0);
+}

[C++ Patch/RFC] PR 67980 ("left shift count is negative [-Wshift-count-negative] generated for unreachable code")

[Paolo Carlini]

Hi,

in the language of our implementations details, submitter noticed that 
in terms of warnings we handle in a different way COND_EXPRs in 
tsubst_copy_and_build - we use fold_non_dependent_expr and integer_zerop 
to suppress undesired warnings by bumping c_inhibit_evaluation_warnings 
- and IF_STMTs in tsubst_expr, where we don't. My patch below, which 
passes testing, tries in a rather straightforward way to adopt the same 
mechanisms in the latter. There are quite a few details I'm not sure 
about: whether we should only use fold_non_dependent_expr for the 
purpose of suppressing the warnings -  thus never touching 'tmp' in the 
pt.c code handling IF_STMTs - which would be completely conservative in 
terms of code generation; whether there are subtle interactions with the 
new if constexpr, which I'm missing at the moment.


Thanks!

Paolo.

//

Index: cp/pt.c
===
--- cp/pt.c (revision 241297)
+++ cp/pt.c (working copy)
@@ -15403,26 +15403,46 @@
   break;
 
 case IF_STMT:
-  stmt = begin_if_stmt ();
-  IF_STMT_CONSTEXPR_P (stmt) = IF_STMT_CONSTEXPR_P (t);
-  tmp = RECUR (IF_COND (t));
-  tmp = finish_if_stmt_cond (tmp, stmt);
-  if (IF_STMT_CONSTEXPR_P (t) && integer_zerop (tmp))
-   /* Don't instantiate the THEN_CLAUSE. */;
-  else
-   RECUR (THEN_CLAUSE (t));
-  finish_then_clause (stmt);
+  {
+   tree folded_tmp;
+   bool zerop, nonzerop;
 
-  if (IF_STMT_CONSTEXPR_P (t) && integer_nonzerop (tmp))
-   /* Don't instantiate the ELSE_CLAUSE. */;
-  else if (ELSE_CLAUSE (t))
-   {
- begin_else_clause (stmt);
- RECUR (ELSE_CLAUSE (t));
- finish_else_clause (stmt);
-   }
+   stmt = begin_if_stmt ();
+   IF_STMT_CONSTEXPR_P (stmt) = IF_STMT_CONSTEXPR_P (t);
+   tmp = RECUR (IF_COND (t));
+   folded_tmp = fold_non_dependent_expr (tmp);
+   if (TREE_CODE (folded_tmp) == INTEGER_CST)
+ tmp = folded_tmp;
+   tmp = finish_if_stmt_cond (tmp, stmt);
+   zerop = integer_zerop (tmp);
+   nonzerop = integer_nonzerop (tmp);
+   if (IF_STMT_CONSTEXPR_P (t) && zerop)
+ /* Don't instantiate the THEN_CLAUSE. */;
+   else
+ {
+   if (zerop)
+ ++c_inhibit_evaluation_warnings;
+   RECUR (THEN_CLAUSE (t));
+   if (zerop)
+ --c_inhibit_evaluation_warnings;
+ }
+   finish_then_clause (stmt);
 
-  finish_if_stmt (stmt);
+   if (IF_STMT_CONSTEXPR_P (t) && nonzerop)
+ /* Don't instantiate the ELSE_CLAUSE. */;
+   else if (ELSE_CLAUSE (t))
+ {
+   begin_else_clause (stmt);
+   if (nonzerop)
+ ++c_inhibit_evaluation_warnings;
+   RECUR (ELSE_CLAUSE (t));
+   if (nonzerop)
+ --c_inhibit_evaluation_warnings;
+   finish_else_clause (stmt);
+ }
+
+   finish_if_stmt (stmt);
+  }
   break;
 
 case BIND_EXPR:
Index: testsuite/g++.dg/cpp1y/pr67980.C
===
--- testsuite/g++.dg/cpp1y/pr67980.C(revision 0)
+++ testsuite/g++.dg/cpp1y/pr67980.C(working copy)
@@ -0,0 +1,23 @@
+// { dg-do compile { target c++14 } }
+
+template 
+constexpr T cpp14_constexpr_then(T value) {
+  if (Y < 0)
+return (value << -Y);
+  else
+return 0;
+}
+
+template 
+constexpr T cpp14_constexpr_else(T value) {
+  if (Y > 0)
+return 0;
+  else
+return (value << -Y);
+}
+
+int main()
+{
+  cpp14_constexpr_then<1>(0);
+  cpp14_constexpr_else<1>(0);
+}