Re: PATCH RFA: C++ frontend: Don't warn about shifts which will not be run

[Ian Lance Taylor <iant at google dot com>]

Jason Merrill <jason@redhat.com> writes:

> On 06/12/2009 02:32 PM, Ian Lance Taylor wrote:
>> This patch modifies the C++ frontend to not warn about bad shift counts
>> for shifts which will never be executed.  It uses the existing
>> skip_evaluation global variable.  It sets the variable when parsing ?:
>> when the condition is known to be true or false.  It only warns about
>> bad shift counts when skip_evaluation is zero.
>
> Hmm...  The use of skip_evaluation in finish_non_static_data_member
> would be incorrect if we start setting it in cases like this.  I think
> this needs a separate flag.

Gosh, and I just asked about that over on gcc@....

I've bootstrapped and tested this patch on x86_64-unknown-linux-gnu.  OK
for mainline?

Ian


gcc/ChangeLog:

2009-06-12  Ian Lance Taylor  <iant@google.com>

	* c-common.c (skip_execution): Define.
	(overflow_warning): Check c_will_be_executed () rather than
	skip_evaluation.
	(convert_and_check, warn_for_div_by_zero): Likewise.
	* c-common.h (skip_execution): Declare.
	(c_will_be_executed): New static inline function.
	* c-parser.c (c_parser_conditional_expression): Adjust
	skip_execution rather than skip_evaluation.
	(c_parser_binary_expression): Likewise.
	* c-typeck.c (build_conditional_expr): Compare skip_evaluation
	with 0 rather than using !.
	(build_binary_op): Check c_will_be_executed () rather than
	skip_evaluation.

gcc/cp/ChangeLog:

2009-06-12  Ian Lance Taylor  <iant@google.com>

	* parser.c (cp_parser_question_colon_clause): Increment
	skip_execution when evaluating an expression which will never be
	executed.
	* cp-tree.h (struct saved_scope): Add skip_execution field.
	* name-lookup.c (push_to_top_level): Handle skip_execution like
	skip_evaluation.
	(pop_from_top_level): Likewise.
	* typeck.c (cp_build_binary_op): Don't warn about bad shift counts
	if the shift will not be executed.
	* pt.c (tsubst_aggr_type): Set skip_evaluation to 0, not false.

gcc/testsuite/ChangeLog:

2009-06-12  Ian Lance Taylor  <iant@google.com>

	* g++.dg/warn/skip-1.C: New testcase.


Index: cp/typeck.c
===================================================================
--- cp/typeck.c	(revision 148327)
+++ cp/typeck.c	(working copy)
@@ -3557,13 +3557,14 @@ cp_build_binary_op (location_t location,
 	    {
 	      if (tree_int_cst_lt (op1, integer_zero_node))
 		{
-		  if (complain & tf_warning)
+		  if ((complain & tf_warning) && c_will_be_executed ())
 		    warning (0, "right shift count is negative");
 		}
 	      else
 		{
 		  if (compare_tree_int (op1, TYPE_PRECISION (type0)) >= 0
-		      && (complain & tf_warning))
+		      && (complain & tf_warning)
+		      && c_will_be_executed ())
 		    warning (0, "right shift count >= width of type");
 		}
 	    }
@@ -3584,12 +3585,12 @@ cp_build_binary_op (location_t location,
 	    {
 	      if (tree_int_cst_lt (op1, integer_zero_node))
 		{
-		  if (complain & tf_warning)
+		  if ((complain & tf_warning) && c_will_be_executed ())
 		    warning (0, "left shift count is negative");
 		}
 	      else if (compare_tree_int (op1, TYPE_PRECISION (type0)) >= 0)
 		{
-		  if (complain & tf_warning)
+		  if ((complain & tf_warning) && c_will_be_executed ())
 		    warning (0, "left shift count >= width of type");
 		}
 	    }
Index: cp/pt.c
===================================================================
--- cp/pt.c	(revision 148327)
+++ cp/pt.c	(working copy)
@@ -7943,7 +7943,7 @@ tsubst_aggr_type (tree t,
 
 	  /* In "sizeof(X<I>)" we need to evaluate "I".  */
 	  saved_skip_evaluation = skip_evaluation;
-	  skip_evaluation = false;
+	  skip_evaluation = 0;
 
 	  /* First, determine the context for the type we are looking
 	     up.  */
Index: cp/parser.c
===================================================================
--- cp/parser.c	(revision 148327)
+++ cp/parser.c	(working copy)
@@ -6385,16 +6385,25 @@ cp_parser_question_colon_clause (cp_pars
   cp_lexer_consume_token (parser->lexer);
   if (cp_parser_allow_gnu_extensions_p (parser)
       && cp_lexer_next_token_is (parser->lexer, CPP_COLON))
-    /* Implicit true clause.  */
-    expr = NULL_TREE;
+    {
+      /* Implicit true clause.  */
+      expr = NULL_TREE;
+      skip_execution += logical_or_expr == truthvalue_true_node;
+    }
   else
-    /* Parse the expression.  */
-    expr = cp_parser_expression (parser, /*cast_p=*/false, NULL);
+    {
+      /* Parse the expression.  */
+      skip_execution += logical_or_expr == truthvalue_false_node;
+      expr = cp_parser_expression (parser, /*cast_p=*/false, NULL);
+      skip_execution += ((logical_or_expr == truthvalue_true_node)
+			 - (logical_or_expr == truthvalue_false_node));
+    }
 
   /* The next token should be a `:'.  */
   cp_parser_require (parser, CPP_COLON, "%<:%>");
   /* Parse the assignment-expression.  */
   assignment_expr = cp_parser_assignment_expression (parser, /*cast_p=*/false, NULL);
+  skip_execution -= logical_or_expr == truthvalue_true_node;
 
   /* Build the conditional-expression.  */
   return build_x_conditional_expr (logical_or_expr,
@@ -18197,7 +18206,7 @@ cp_parser_enclosed_template_argument_lis
   /* We need to evaluate the template arguments, even though this
      template-id may be nested within a "sizeof".  */
   saved_skip_evaluation = skip_evaluation;
-  skip_evaluation = false;
+  skip_evaluation = 0;
   /* Parse the template-argument-list itself.  */
   if (cp_lexer_next_token_is (parser->lexer, CPP_GREATER)
       || cp_lexer_next_token_is (parser->lexer, CPP_RSHIFT))
Index: cp/cp-tree.h
===================================================================
--- cp/cp-tree.h	(revision 148327)
+++ cp/cp-tree.h	(working copy)
@@ -756,6 +756,7 @@ struct GTY(()) saved_scope {
   BOOL_BITFIELD x_processing_explicit_instantiation : 1;
   BOOL_BITFIELD need_pop_function_context : 1;
   BOOL_BITFIELD skip_evaluation : 1;
+  BOOL_BITFIELD skip_execution : 1;
 
   struct stmt_tree_s x_stmt_tree;
 
Index: cp/name-lookup.c
===================================================================
--- cp/name-lookup.c	(revision 148327)
+++ cp/name-lookup.c	(working copy)
@@ -5320,6 +5320,7 @@ push_to_top_level (void)
   s->need_pop_function_context = need_pop;
   s->function_decl = current_function_decl;
   s->skip_evaluation = skip_evaluation;
+  s->skip_execution = skip_execution;
 
   scope_chain = s;
   current_function_decl = NULL_TREE;
@@ -5328,6 +5329,7 @@ push_to_top_level (void)
   current_namespace = global_namespace;
   push_class_stack ();
   skip_evaluation = 0;
+  skip_execution = 0;
   timevar_pop (TV_NAME_LOOKUP);
 }
 
@@ -5361,6 +5363,7 @@ pop_from_top_level (void)
     pop_function_context ();
   current_function_decl = s->function_decl;
   skip_evaluation = s->skip_evaluation;
+  skip_execution = s->skip_execution;
   timevar_pop (TV_NAME_LOOKUP);
 }
 
Index: c-typeck.c
===================================================================
--- c-typeck.c	(revision 148327)
+++ c-typeck.c	(working copy)
@@ -3853,7 +3853,7 @@ build_conditional_expr (location_t colon
 	 and later code won't know it used to be different.
 	 Do this check on the original types, so that explicit casts
 	 will be considered, but default promotions won't.  */
-      if (!skip_evaluation)
+      if (skip_evaluation == 0)
 	{
 	  int unsigned_op1 = TYPE_UNSIGNED (TREE_TYPE (orig_op1));
 	  int unsigned_op2 = TYPE_UNSIGNED (TREE_TYPE (orig_op2));
@@ -9169,7 +9169,7 @@ build_binary_op (location_t location, en
 	      if (tree_int_cst_sgn (op1) < 0)
 		{
 		  int_const = false;
-		  if (skip_evaluation == 0)
+		  if (c_will_be_executed ())
 		    warning (0, "right shift count is negative");
 		}
 	      else
@@ -9180,7 +9180,7 @@ build_binary_op (location_t location, en
 		  if (compare_tree_int (op1, TYPE_PRECISION (type0)) >= 0)
 		    {
 		      int_const = false;
-		      if (skip_evaluation == 0)
+		      if (c_will_be_executed ())
 			warning (0, "right shift count >= width of type");
 		    }
 		}
@@ -9206,14 +9206,14 @@ build_binary_op (location_t location, en
 	      if (tree_int_cst_sgn (op1) < 0)
 		{
 		  int_const = false;
-		  if (skip_evaluation == 0)
+		  if (c_will_be_executed ())
 		    warning (0, "left shift count is negative");
 		}
 
 	      else if (compare_tree_int (op1, TYPE_PRECISION (type0)) >= 0)
 		{
 		  int_const = false;
-		  if (skip_evaluation == 0)
+		  if (c_will_be_executed ())
 		    warning (0, "left shift count >= width of type");
 		}
 	    }
Index: c-common.c
===================================================================
--- c-common.c	(revision 148327)
+++ c-common.c	(working copy)
@@ -442,10 +442,14 @@ tree *ridpointers;
 
 tree (*make_fname_decl) (tree, int);
 
-/* Nonzero means the expression being parsed will never be evaluated.
-   This is a count, since unevaluated expressions can nest.  */
+/* Nonzero means the expression being parsed will never be
+   evaluated.  */
 int skip_evaluation;
 
+/* Nonzero means that the expression being parsed will never be
+   executed.  */
+int skip_execution;
+
 /* Whether lexing has been completed, so subsequent preprocessor
    errors should use the compiler's input_location.  */
 bool done_lexing = false;
@@ -1507,7 +1511,8 @@ constant_expression_error (tree value)
 void
 overflow_warning (tree value)
 {
-  if (skip_evaluation) return;
+  if (!c_will_be_executed ())
+    return;
 
   switch (TREE_CODE (value))
     {
@@ -2222,7 +2227,9 @@ convert_and_check (tree type, tree expr)
   
   result = convert (type, expr);
 
-  if (!skip_evaluation && !TREE_OVERFLOW_P (expr) && result != error_mark_node)
+  if (c_will_be_executed ()
+      && !TREE_OVERFLOW_P (expr)
+      && result != error_mark_node)
     warnings_for_convert_and_check (type, expr_for_warning, result);
 
   return result;
@@ -8899,7 +8906,7 @@ warn_for_div_by_zero (location_t loc, tr
      about division by zero.  Do not issue a warning if DIVISOR has a
      floating-point type, since we consider 0.0/0.0 a valid way of
      generating a NaN.  */
-  if (skip_evaluation == 0
+  if (c_will_be_executed ()
       && (integer_zerop (divisor) || fixed_zerop (divisor)))
     warning_at (loc, OPT_Wdiv_by_zero, "division by zero");
 }
Index: c-common.h
===================================================================
--- c-common.h	(revision 148327)
+++ c-common.h	(working copy)
@@ -720,10 +720,25 @@ extern int warn_strict_null_sentinel;
 extern int max_tinst_depth;
 
 /* Nonzero means the expression being parsed will never be evaluated.
-   This is a count, since unevaluated expressions can nest.  */
+   This is a count, since unevaluated expressions can nest.  This is
+   used for cases like sizeof().  */
 
 extern int skip_evaluation;
 
+/* Nonzero means that the expression being parsed will never be
+   executed.  This is a count, since unexecuted expressions can nest.
+   This is for cases like "0 ? a : b".  */
+
+extern int skip_execution;
+
+/* Return true if things currently being parsed will be executed.  */
+
+static inline bool
+c_will_be_executed (void)
+{
+  return skip_evaluation == 0 && skip_execution == 0;
+}
+
 /* Whether lexing has been completed, so subsequent preprocessor
    errors should use the compiler's input_location.  */
 
Index: c-parser.c
===================================================================
--- c-parser.c	(revision 148327)
+++ c-parser.c	(working copy)
@@ -4530,23 +4530,23 @@ c_parser_conditional_expression (c_parse
 	exp1.value = build1 (EXCESS_PRECISION_EXPR, eptype, exp1.value);
       exp1.original_type = NULL;
       cond.value = c_objc_common_truthvalue_conversion (cond_loc, exp1.value);
-      skip_evaluation += cond.value == truthvalue_true_node;
+      skip_execution += cond.value == truthvalue_true_node;
     }
   else
     {
       cond.value
 	= c_objc_common_truthvalue_conversion
 	(cond_loc, default_conversion (cond.value));
-      skip_evaluation += cond.value == truthvalue_false_node;
+      skip_execution += cond.value == truthvalue_false_node;
       exp1 = c_parser_expression_conv (parser);
-      skip_evaluation += ((cond.value == truthvalue_true_node)
-			  - (cond.value == truthvalue_false_node));
+      skip_execution += ((cond.value == truthvalue_true_node)
+			 - (cond.value == truthvalue_false_node));
     }
 
   colon_loc = c_parser_peek_token (parser)->location;
   if (!c_parser_require (parser, CPP_COLON, "expected %<:%>"))
     {
-      skip_evaluation -= cond.value == truthvalue_true_node;
+      skip_execution -= cond.value == truthvalue_true_node;
       ret.value = error_mark_node;
       ret.original_code = ERROR_MARK;
       ret.original_type = NULL;
@@ -4554,7 +4554,7 @@ c_parser_conditional_expression (c_parse
     }
   exp2 = c_parser_conditional_expression (parser, NULL);
   exp2 = default_function_array_conversion (exp2);
-  skip_evaluation -= cond.value == truthvalue_true_node;
+  skip_execution -= cond.value == truthvalue_true_node;
   ret.value = build_conditional_expr (colon_loc, cond.value,
 				      cond.original_code == C_MAYBE_CONST_EXPR,
 				      exp1.value, exp2.value);
@@ -4655,7 +4655,7 @@ c_parser_binary_expression (c_parser *pa
      the stack has lower precedence than the new operator or there is
      only one element on the stack; then the top expression is the LHS
      of the new operator.  In the case of logical AND and OR
-     expressions, we also need to adjust skip_evaluation as
+     expressions, we also need to adjust skip_execution as
      appropriate when the operators are pushed and popped.  */
 
   /* The precedence levels, where 0 is a dummy lowest level used for
@@ -4693,10 +4693,10 @@ c_parser_binary_expression (c_parser *pa
     switch (stack[sp].op)						      \
       {									      \
       case TRUTH_ANDIF_EXPR:						      \
-	skip_evaluation -= stack[sp - 1].expr.value == truthvalue_false_node; \
+	skip_execution -= stack[sp - 1].expr.value == truthvalue_false_node;  \
 	break;								      \
       case TRUTH_ORIF_EXPR:						      \
-	skip_evaluation -= stack[sp - 1].expr.value == truthvalue_true_node;  \
+	skip_execution -= stack[sp - 1].expr.value == truthvalue_true_node;   \
 	break;								      \
       default:								      \
 	break;								      \
@@ -4812,14 +4812,14 @@ c_parser_binary_expression (c_parser *pa
 	    = default_function_array_conversion (stack[sp].expr);
 	  stack[sp].expr.value = c_objc_common_truthvalue_conversion
 	    (stack[sp].loc, default_conversion (stack[sp].expr.value));
-	  skip_evaluation += stack[sp].expr.value == truthvalue_false_node;
+	  skip_execution += stack[sp].expr.value == truthvalue_false_node;
 	  break;
 	case TRUTH_ORIF_EXPR:
 	  stack[sp].expr
 	    = default_function_array_conversion (stack[sp].expr);
 	  stack[sp].expr.value = c_objc_common_truthvalue_conversion
 	    (stack[sp].loc, default_conversion (stack[sp].expr.value));
-	  skip_evaluation += stack[sp].expr.value == truthvalue_true_node;
+	  skip_execution += stack[sp].expr.value == truthvalue_true_node;
 	  break;
 	default:
 	  break;
Index: testsuite/g++.dg/warn/skip-1.C
===================================================================
--- testsuite/g++.dg/warn/skip-1.C	(revision 0)
+++ testsuite/g++.dg/warn/skip-1.C	(revision 0)
@@ -0,0 +1,17 @@
+// { dg-do compile }
+// { dg-options "-Wall" }
+
+// Check that we don't warn about code that will not be executed.
+extern int f2(int);
+void
+f1(int i)
+{
+  f2(1 == 1 ? 0 : f2(i >> -10));
+  f2(1 == 1 ? 0 : f2(i >> 128));
+  f2(1 == 1 ? 0 : f2(i << -10));
+  f2(1 == 1 ? 0 : f2(1 << 128));
+  f2(1 != 1 ? f2(i >> -10) : 0);
+  f2(1 != 1 ? f2(i >> 128) : 0);
+  f2(1 != 1 ? f2(i << -10) : 0);
+  f2(1 != 1 ? f2(1 << 128) : 0);
+}