diff mbox

[/,RFC] Improving more locations for binary expressions

Message ID 4FADA1FD.1050904@oracle.com
State New
Headers show

Commit Message

Paolo Carlini May 11, 2012, 11:34 p.m. UTC
On 05/10/2012 04:28 PM, Jason Merrill wrote:
> Looks good.
Thanks Jason. The below is the idea fully implemented.

The call.c bits are exactly in the form I had in mind a couple of days ago.

The parser.c bits, the ones I actually preliminarily posted, are now a 
bit different: I noticed that in the middle of the function we were 
reading / writing all the cp_parser_expression_stack_entry fields by 
hand (and now would be one more) thus tweaked the algorithm to use a 
cp_parser_expression_stack_entry as local variable. In my opinion this 
tidies the code quite a bit and also makes manifest that tree_type and 
loc are always updated at the same time, as they should be.

Booted and tested x86_64-linux. Ok?

Thanks,
Paolo.

//////////////////////////////
/cp
2012-05-12

	* parser.c (struct cp_parser_expression_stack_entry): Add location_t
	field.
	(cp_parser_binary_expression): Rework to always update at the same
	time tree_type and loc.
	* call.c (print_z_candidate): Add location_t parameter.
	(print_z_candidates, convert_like_real, joust): Adjust.

/testsuite
2012-05-12

	* g++.dg/parse/error47.C: New.

Comments

Jason Merrill May 12, 2012, 3:13 a.m. UTC | #1
I find the name "sentry" confusing; I don't see how it applies.  Perhaps 
"current" instead?

Otherwise, the patch is OK.

Jason
diff mbox

Patch

Index: testsuite/g++.dg/parse/error47.C
===================================================================
--- testsuite/g++.dg/parse/error47.C	(revision 0)
+++ testsuite/g++.dg/parse/error47.C	(revision 0)
@@ -0,0 +1,9 @@ 
+struct T { };
+
+T foo();
+
+void bar(int a, int b)
+{
+  if (foo() && a < b) // { dg-error "13:no match for 'operator&&'" }
+    ;
+}
Index: cp/parser.c
===================================================================
--- cp/parser.c	(revision 187414)
+++ cp/parser.c	(working copy)
@@ -1621,6 +1621,8 @@  typedef struct cp_parser_expression_stack_entry
   enum tree_code tree_type;
   /* Precedence of the binary operation we are parsing.  */
   enum cp_parser_prec prec;
+  /* Location of the binary operation we are parsing.  */
+  location_t loc;
 } cp_parser_expression_stack_entry;
 
 /* The stack for storing partial expressions.  We only need NUM_PREC_VALUES
@@ -7275,30 +7277,33 @@  cp_parser_binary_expression (cp_parser* parser, bo
 {
   cp_parser_expression_stack stack;
   cp_parser_expression_stack_entry *sp = &stack[0];
-  tree lhs, rhs;
+  cp_parser_expression_stack_entry sentry;
+  tree rhs;
   cp_token *token;
-  location_t loc;
-  enum tree_code tree_type, lhs_type, rhs_type;
+  enum tree_code rhs_type;
   enum cp_parser_prec new_prec, lookahead_prec;
   tree overload;
 
   /* Parse the first expression.  */
-  lhs = cp_parser_cast_expression (parser, /*address_p=*/false, cast_p, pidk);
-  lhs_type = ERROR_MARK;
+  sentry.lhs = cp_parser_cast_expression (parser, /*address_p=*/false,
+					  cast_p, pidk);
+  sentry.lhs_type = ERROR_MARK;
+  sentry.prec = prec;
 
   for (;;)
     {
       /* Get an operator token.  */
       token = cp_lexer_peek_token (parser->lexer);
-      loc = token->location;
 
       if (warn_cxx0x_compat
           && token->type == CPP_RSHIFT
           && !parser->greater_than_is_operator_p)
         {
-          if (warning_at (loc, OPT_Wc__0x_compat, "%<>>%> operator is treated"
+          if (warning_at (token->location, OPT_Wc__0x_compat,
+			  "%<>>%> operator is treated"
 			  " as two right angle brackets in C++11"))
-	    inform (loc, "suggest parentheses around %<>>%> expression");
+	    inform (token->location,
+		    "suggest parentheses around %<>>%> expression");
         }
 
       new_prec = TOKEN_PRECEDENCE (token);
@@ -7310,7 +7315,7 @@  cp_parser_binary_expression (cp_parser* parser, bo
 	 - or, we found an operator which has lower priority.  This is the case
 	   where the recursive descent *ascends*, as in `3 * 4 + 5' after
 	   parsing `3 * 4'.  */
-      if (new_prec <= prec)
+      if (new_prec <= sentry.prec)
 	{
 	  if (sp == stack)
 	    break;
@@ -7319,17 +7324,18 @@  cp_parser_binary_expression (cp_parser* parser, bo
 	}
 
      get_rhs:
-      tree_type = binops_by_token[token->type].tree_type;
+      sentry.tree_type = binops_by_token[token->type].tree_type;
+      sentry.loc = token->location;
 
       /* We used the operator token.  */
       cp_lexer_consume_token (parser->lexer);
 
       /* For "false && x" or "true || x", x will never be executed;
 	 disable warnings while evaluating it.  */
-      if (tree_type == TRUTH_ANDIF_EXPR)
-	c_inhibit_evaluation_warnings += lhs == truthvalue_false_node;
-      else if (tree_type == TRUTH_ORIF_EXPR)
-	c_inhibit_evaluation_warnings += lhs == truthvalue_true_node;
+      if (sentry.tree_type == TRUTH_ANDIF_EXPR)
+	c_inhibit_evaluation_warnings += sentry.lhs == truthvalue_false_node;
+      else if (sentry.tree_type == TRUTH_ORIF_EXPR)
+	c_inhibit_evaluation_warnings += sentry.lhs == truthvalue_true_node;
 
       /* Extract another operand.  It may be the RHS of this expression
 	 or the LHS of a new, higher priority expression.  */
@@ -7347,14 +7353,11 @@  cp_parser_binary_expression (cp_parser* parser, bo
 	     expression.  Since precedence levels on the stack are
 	     monotonically increasing, we do not have to care about
 	     stack overflows.  */
-	  sp->prec = prec;
-	  sp->tree_type = tree_type;
-	  sp->lhs = lhs;
-	  sp->lhs_type = lhs_type;
-	  sp++;
-	  lhs = rhs;
-	  lhs_type = rhs_type;
-	  prec = new_prec;
+	  *sp = sentry;
+	  ++sp;
+	  sentry.lhs = rhs;
+	  sentry.lhs_type = rhs_type;
+	  sentry.prec = new_prec;
 	  new_prec = lookahead_prec;
 	  goto get_rhs;
 
@@ -7367,20 +7370,17 @@  cp_parser_binary_expression (cp_parser* parser, bo
 	     the precedence of the higher level subexpression
 	     (`PREC_ADDITIVE_EXPRESSION').  TOKEN is the CPP_PLUS token,
 	     which will be used to actually build the additive expression.  */
+	  rhs = sentry.lhs;
+	  rhs_type = sentry.lhs_type;
 	  --sp;
-	  prec = sp->prec;
-	  tree_type = sp->tree_type;
-	  rhs = lhs;
-	  rhs_type = lhs_type;
-	  lhs = sp->lhs;
-	  lhs_type = sp->lhs_type;
+	  sentry = *sp;
 	}
 
       /* Undo the disabling of warnings done above.  */
-      if (tree_type == TRUTH_ANDIF_EXPR)
-	c_inhibit_evaluation_warnings -= lhs == truthvalue_false_node;
-      else if (tree_type == TRUTH_ORIF_EXPR)
-	c_inhibit_evaluation_warnings -= lhs == truthvalue_true_node;
+      if (sentry.tree_type == TRUTH_ANDIF_EXPR)
+	c_inhibit_evaluation_warnings -= sentry.lhs == truthvalue_false_node;
+      else if (sentry.tree_type == TRUTH_ORIF_EXPR)
+	c_inhibit_evaluation_warnings -= sentry.lhs == truthvalue_true_node;
 
       overload = NULL;
       /* ??? Currently we pass lhs_type == ERROR_MARK and rhs_type ==
@@ -7391,14 +7391,17 @@  cp_parser_binary_expression (cp_parser* parser, bo
 	 surrounded by parentheses.
       */
       if (no_toplevel_fold_p
-	  && lookahead_prec <= prec
+	  && lookahead_prec <= sentry.prec
 	  && sp == stack
-	  && TREE_CODE_CLASS (tree_type) == tcc_comparison)
-	lhs = build2 (tree_type, boolean_type_node, lhs, rhs);
+	  && TREE_CODE_CLASS (sentry.tree_type) == tcc_comparison)
+	sentry.lhs = build2 (sentry.tree_type, boolean_type_node,
+			     sentry.lhs, rhs);
       else
-	lhs = build_x_binary_op (loc, tree_type, lhs, lhs_type, rhs, rhs_type,
-				 &overload, tf_warning_or_error);
-      lhs_type = tree_type;
+	sentry.lhs = build_x_binary_op (sentry.loc, sentry.tree_type,
+					sentry.lhs, sentry.lhs_type,
+					rhs, rhs_type, &overload,
+					tf_warning_or_error);
+      sentry.lhs_type = sentry.tree_type;
 
       /* If the binary operator required the use of an overloaded operator,
 	 then this expression cannot be an integral constant-expression.
@@ -7412,7 +7415,7 @@  cp_parser_binary_expression (cp_parser* parser, bo
 	return error_mark_node;
     }
 
-  return lhs;
+  return sentry.lhs;
 }
 
 
Index: cp/call.c
===================================================================
--- cp/call.c	(revision 187414)
+++ cp/call.c	(working copy)
@@ -163,7 +163,7 @@  static void op_error (location_t, enum tree_code,
 		      tree, tree, bool);
 static struct z_candidate *build_user_type_conversion_1 (tree, tree, int,
 							 tsubst_flags_t);
-static void print_z_candidate (const char *, struct z_candidate *);
+static void print_z_candidate (location_t, const char *, struct z_candidate *);
 static void print_z_candidates (location_t, struct z_candidate *);
 static tree build_this (tree);
 static struct z_candidate *splice_viable (struct z_candidate *, bool, bool *);
@@ -3159,36 +3159,38 @@  print_arity_information (location_t loc, unsigned
    life simpler in print_z_candidates and for the translators.  */
 
 static void
-print_z_candidate (const char *msgstr, struct z_candidate *candidate)
+print_z_candidate (location_t loc, const char *msgstr,
+		   struct z_candidate *candidate)
 {
   const char *msg = (msgstr == NULL
 		     ? ""
 		     : ACONCAT ((msgstr, " ", NULL)));
-  location_t loc = location_of (candidate->fn);
+  location_t cloc = location_of (candidate->fn);
 
   if (TREE_CODE (candidate->fn) == IDENTIFIER_NODE)
     {
+      cloc = loc;
       if (candidate->num_convs == 3)
-	inform (input_location, "%s%D(%T, %T, %T) <built-in>", msg, candidate->fn,
+	inform (cloc, "%s%D(%T, %T, %T) <built-in>", msg, candidate->fn,
 		candidate->convs[0]->type,
 		candidate->convs[1]->type,
 		candidate->convs[2]->type);
       else if (candidate->num_convs == 2)
-	inform (input_location, "%s%D(%T, %T) <built-in>", msg, candidate->fn,
+	inform (cloc, "%s%D(%T, %T) <built-in>", msg, candidate->fn,
 		candidate->convs[0]->type,
 		candidate->convs[1]->type);
       else
-	inform (input_location, "%s%D(%T) <built-in>", msg, candidate->fn,
+	inform (cloc, "%s%D(%T) <built-in>", msg, candidate->fn,
 		candidate->convs[0]->type);
     }
   else if (TYPE_P (candidate->fn))
-    inform (loc, "%s%T <conversion>", msg, candidate->fn);
+    inform (cloc, "%s%T <conversion>", msg, candidate->fn);
   else if (candidate->viable == -1)
-    inform (loc, "%s%#D <near match>", msg, candidate->fn);
+    inform (cloc, "%s%#D <near match>", msg, candidate->fn);
   else if (DECL_DELETED_FN (STRIP_TEMPLATE (candidate->fn)))
-    inform (loc, "%s%#D <deleted>", msg, candidate->fn);
+    inform (cloc, "%s%#D <deleted>", msg, candidate->fn);
   else
-    inform (loc, "%s%#D", msg, candidate->fn);
+    inform (cloc, "%s%#D", msg, candidate->fn);
   /* Give the user some information about why this candidate failed.  */
   if (candidate->reason != NULL)
     {
@@ -3197,23 +3199,23 @@  static void
       switch (r->code)
 	{
 	case rr_arity:
-	  print_arity_information (loc, r->u.arity.actual,
+	  print_arity_information (cloc, r->u.arity.actual,
 				   r->u.arity.expected);
 	  break;
 	case rr_arg_conversion:
-	  print_conversion_rejection (loc, &r->u.conversion);
+	  print_conversion_rejection (cloc, &r->u.conversion);
 	  break;
 	case rr_bad_arg_conversion:
-	  print_conversion_rejection (loc, &r->u.bad_conversion);
+	  print_conversion_rejection (cloc, &r->u.bad_conversion);
 	  break;
 	case rr_explicit_conversion:
-	  inform (loc, "  return type %qT of explicit conversion function "
+	  inform (cloc, "  return type %qT of explicit conversion function "
 		  "cannot be converted to %qT with a qualification "
 		  "conversion", r->u.conversion.from_type,
 		  r->u.conversion.to_type);
 	  break;
 	case rr_template_conversion:
-	  inform (loc, "  conversion from return type %qT of template "
+	  inform (cloc, "  conversion from return type %qT of template "
 		  "conversion function specialization to %qT is not an "
 		  "exact match", r->u.conversion.from_type,
 		  r->u.conversion.to_type);
@@ -3224,12 +3226,12 @@  static void
 	     them here.  */
 	  if (r->u.template_unification.tmpl == NULL_TREE)
 	    {
-	      inform (loc, "  substitution of deduced template arguments "
+	      inform (cloc, "  substitution of deduced template arguments "
 		      "resulted in errors seen above");
 	      break;
 	    }
 	  /* Re-run template unification with diagnostics.  */
-	  inform (loc, "  template argument deduction/substitution failed:");
+	  inform (cloc, "  template argument deduction/substitution failed:");
 	  fn_type_unification (r->u.template_unification.tmpl,
 			       r->u.template_unification.explicit_targs,
 			       r->u.template_unification.targs,
@@ -3247,7 +3249,7 @@  static void
 				tf_warning_or_error);
 	  break;
 	case rr_invalid_copy:
-	  inform (loc,
+	  inform (cloc,
 		  "  a constructor taking a single argument of its own "
 		  "class type is invalid");
 	  break;
@@ -3312,7 +3314,7 @@  print_z_candidates (location_t loc, struct z_candi
 
   inform_n (loc, n_candidates, "candidate is:", "candidates are:");
   for (; candidates; candidates = candidates->next)
-    print_z_candidate (NULL, candidates);
+    print_z_candidate (loc, NULL, candidates);
 }
 
 /* USER_SEQ is a user-defined conversion sequence, beginning with a
@@ -5678,7 +5680,7 @@  convert_like_real (conversion *convs, tree expr, t
 	    {
 	      permerror (loc, "invalid user-defined conversion "
 			 "from %qT to %qT", TREE_TYPE (expr), totype);
-	      print_z_candidate ("candidate is:", t->cand);
+	      print_z_candidate (loc, "candidate is:", t->cand);
 	      expr = convert_like_real (t, expr, fn, argnum, 1,
 					/*issue_conversion_warnings=*/false,
 					/*c_cast_p=*/false,
@@ -8405,8 +8407,8 @@  tweak:
 	      "ISO C++ says that these are ambiguous, even "
 	      "though the worst conversion for the first is better than "
 	      "the worst conversion for the second:");
-	      print_z_candidate (_("candidate 1:"), w);
-	      print_z_candidate (_("candidate 2:"), l);
+	      print_z_candidate (input_location, _("candidate 1:"), w);
+	      print_z_candidate (input_location, _("candidate 2:"), l);
 	    }
 	  else
 	    add_warning (w, l);