diff mbox series

[C++] Fix c++/84661, c++/85013, and other recovery regressions in cp_parser_binary_expression

Message ID 8898d175-7a24-d9a5-9391-d1830f7fb2b0@oracle.com
State New
Headers show
Series [C++] Fix c++/84661, c++/85013, and other recovery regressions in cp_parser_binary_expression | expand

Commit Message

Paolo Carlini March 25, 2019, 9:28 a.m. UTC 
Hi,

a while ago I noticed that in cp_parser_binary_expression we were 
calling cp_fully_fold first to disable the warnings for not executed 
expressions, and then, seemingly in a very redundant and inconsistent 
with other situations way, to re-enable the warnings (*). Thus, I meant 
to investigate opportunities for a mini optimization / clean-up during 
the next Stage 1.

However, over the last couple of weeks I noticed that we had in Bugzilla 
a number of error-recovery regressions happening starting from the 
*second* cp_fully_fold calls, those which likely could be completely 
avoided. Note, not only we were calling again cp_fully_fold, we were 
also doing that for expressions which could not be folded to true/false 
the first time we tried, error-prone and wasteful too.

Then the below which seems rather straightforward to me. To be 
super-safe I carried out a number of additional checks and instrumented 
testsuite runs: that we exercise the code enough; that when we set 
disable_warnings_sp = sp we find it NULL, etc. Everything went well..

Tested x86_64-linux.

Thanks, Paolo.

(*) Historically, we used not to have this disabling code at all, then a 
very basic version not folding.

/////////////////
/cp
2019-03-25  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/84661
	PR c++/85013
	* parser.c (cp_parser_binary_expression): Don't call cp_fully_fold
	to undo the disabling of warnings.

/testsuite
2019-03-25  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/84661
	PR c++/85013
	* g++.dg/concepts/pr84661.C: New.
	* g++.dg/torture/pr85013.C: Likewise.

Comments

Jason Merrill March 25, 2019, 4:20 p.m. UTC | #1 
On 3/25/19 5:28 AM, Paolo Carlini wrote:
> Hi,
> 
> a while ago I noticed that in cp_parser_binary_expression we were 
> calling cp_fully_fold first to disable the warnings for not executed 
> expressions, and then, seemingly in a very redundant and inconsistent 
> with other situations way, to re-enable the warnings (*). Thus, I meant 
> to investigate opportunities for a mini optimization / clean-up during 
> the next Stage 1.
> 
> However, over the last couple of weeks I noticed that we had in Bugzilla 
> a number of error-recovery regressions happening starting from the 
> *second* cp_fully_fold calls, those which likely could be completely 
> avoided. Note, not only we were calling again cp_fully_fold, we were 
> also doing that for expressions which could not be folded to true/false 
> the first time we tried, error-prone and wasteful too.
> 
> Then the below which seems rather straightforward to me. To be 
> super-safe I carried out a number of additional checks and instrumented 
> testsuite runs: that we exercise the code enough; that when we set 
> disable_warnings_sp = sp we find it NULL, etc. Everything went well..
> 
> Tested x86_64-linux.
> 
> Thanks, Paolo.
> 
> (*) Historically, we used not to have this disabling code at all, then a 
> very basic version not folding.
> 
> /////////////////
> 
> 

OK.

Jason
diff mbox series

Patch

Index: cp/parser.c
===================================================================
--- cp/parser.c	(revision 269881)
+++ cp/parser.c	(working copy)
@@ -9443,6 +9443,7 @@  cp_parser_binary_expression (cp_parser* parser, bo
 {
   cp_parser_expression_stack stack;
   cp_parser_expression_stack_entry *sp = &stack[0];
+  cp_parser_expression_stack_entry *disable_warnings_sp = NULL;
   cp_parser_expression_stack_entry current;
   cp_expr rhs;
   cp_token *token;
@@ -9506,12 +9507,14 @@  cp_parser_binary_expression (cp_parser* parser, bo
 
       /* For "false && x" or "true || x", x will never be executed;
 	 disable warnings while evaluating it.  */
-      if (current.tree_type == TRUTH_ANDIF_EXPR)
-	c_inhibit_evaluation_warnings +=
-	  cp_fully_fold (current.lhs) == truthvalue_false_node;
-      else if (current.tree_type == TRUTH_ORIF_EXPR)
-	c_inhibit_evaluation_warnings +=
-	  cp_fully_fold (current.lhs) == truthvalue_true_node;
+      if ((current.tree_type == TRUTH_ANDIF_EXPR
+	   && cp_fully_fold (current.lhs) == truthvalue_false_node)
+	  || (current.tree_type == TRUTH_ORIF_EXPR
+	      && cp_fully_fold (current.lhs) == truthvalue_true_node))
+	{
+	  disable_warnings_sp = sp;
+	  ++c_inhibit_evaluation_warnings;
+	}
 
       /* Extract another operand.  It may be the RHS of this expression
 	 or the LHS of a new, higher priority expression.  */
@@ -9557,12 +9560,11 @@  cp_parser_binary_expression (cp_parser* parser, bo
 	}
 
       /* Undo the disabling of warnings done above.  */
-      if (current.tree_type == TRUTH_ANDIF_EXPR)
-	c_inhibit_evaluation_warnings -=
-	  cp_fully_fold (current.lhs) == truthvalue_false_node;
-      else if (current.tree_type == TRUTH_ORIF_EXPR)
-	c_inhibit_evaluation_warnings -=
-	  cp_fully_fold (current.lhs) == truthvalue_true_node;
+      if (sp == disable_warnings_sp)
+	{
+	  disable_warnings_sp = NULL;
+	  --c_inhibit_evaluation_warnings;
+	}
 
       if (warn_logical_not_paren
 	  && TREE_CODE_CLASS (current.tree_type) == tcc_comparison
Index: testsuite/g++.dg/concepts/pr84661.C
===================================================================
--- testsuite/g++.dg/concepts/pr84661.C	(nonexistent)
+++ testsuite/g++.dg/concepts/pr84661.C	(working copy)
@@ -0,0 +1,7 @@ 
+// { dg-do compile { target c++14 } }
+// { dg-additional-options "-fconcepts" }
+
+struct S {
+  int &a;
+  void foo (decltype(((a = 0) || ((auto)))));  // { dg-error "expected" }
+};
Index: testsuite/g++.dg/torture/pr85013.C
===================================================================
--- testsuite/g++.dg/torture/pr85013.C	(nonexistent)
+++ testsuite/g++.dg/torture/pr85013.C	(working copy)
@@ -0,0 +1,3 @@ 
+// { dg-additional-options "-std=c++14 -fconcepts" }
+
+a(decltype((0 > 1e91 && 1e31 && (auto))));  // { dg-error "expected" }