diff mbox

[C++] PR 44516

Message ID 4FB07AF9.8050500@oracle.com
State New
Headers show

Commit Message

Paolo Carlini May 14, 2012, 3:24 a.m. UTC 
Hi,

On 05/14/2012 03:50 AM, Jason Merrill wrote:
> On 05/13/2012 07:25 PM, Paolo Carlini wrote:
>
> You're adding a lot of uses of input_location; I think we can do better.
>
>>         if (TREE_CODE (incr) == MODIFY_EXPR)
>> -    incr = build_x_modify_expr (RECUR (TREE_OPERAND (incr, 0)), 
>> NOP_EXPR,
>> +    incr = build_x_modify_expr (input_location,
>
> EXPR_LOC_OR_HERE (incr), I would think.
Ok.
>
>>      tree r = build_x_modify_expr
>> -      (RECUR (TREE_OPERAND (t, 0)),
>> +      (input_location,
>
> And EXPR_LOC_OR_HERE (t).
Here I think EXPR_LOC_OR_HERE (TREE_OPERAND (t, 1)) is better.
>
>> -          iter_incr = build_x_modify_expr (iter, TREE_CODE (rhs),
>> +          iter_incr = build_x_modify_expr (input_location,
>> +                           iter, TREE_CODE (rhs),
>>                             TREE_OPERAND (rhs, 1),
>
> And EXPR_LOC_OR_HERE (rhs).
>
> And so on.
Agreed, sorry about a bit of sloppiness on my part, I didn't see any 
obvious "expr" very close and gave up too quickly (because wasn't 
risking regressions ;) Now I went through all of them and also fixed two 
or three input_location which I added days ago in build_x_binary_op 
calls. Also, in the case of the code synthesized in the final part of 
handle_omp_for_class_iterator Jakub had already prepared elocus (and 
used it in error_at).

Anyway, the below is finishing testing. Ok if it passes?

Thanks,
Paolo.

///////////////////////////////

Comments

Jason Merrill May 14, 2012, 3:58 a.m. UTC | #1 
On 05/13/2012 11:24 PM, Paolo Carlini wrote:
>>>      tree r = build_x_modify_expr
>>> -      (RECUR (TREE_OPERAND (t, 0)),
>>> +      (input_location,
>>
>> And EXPR_LOC_OR_HERE (t).
> Here I think EXPR_LOC_OR_HERE (TREE_OPERAND (t, 1)) is better.

Why?  TREE_OPERAND (t,1) is a dummy tree that we only use for its code.

Of course, currently it doesn't matter because we don't 
SET_EXPR_LOCATION on either the MODOP_EXPR or its operand 1, so 
EXPR_LOC_OR_HERE on either one will give input_location.

I guess we want a build_min_nt_loc function.

Jason
Paolo Carlini May 14, 2012, 1:22 p.m. UTC | #2 
Hi,

On 05/14/2012 05:58 AM, Jason Merrill wrote:
> On 05/13/2012 11:24 PM, Paolo Carlini wrote:
>>>>      tree r = build_x_modify_expr
>>>> -      (RECUR (TREE_OPERAND (t, 0)),
>>>> +      (input_location,
>>>
>>> And EXPR_LOC_OR_HERE (t).
>> Here I think EXPR_LOC_OR_HERE (TREE_OPERAND (t, 1)) is better.
>
> Why?  TREE_OPERAND (t,1) is a dummy tree that we only use for its code.
Ok... now I see.
> Of course, currently it doesn't matter because we don't 
> SET_EXPR_LOCATION on either the MODOP_EXPR or its operand 1, so 
> EXPR_LOC_OR_HERE on either one will give input_location.
Agreed.
> I guess we want a build_min_nt_loc function.
Indeed, and I can use it to handle the latter issue. I have a draft 
patch almost ready.

In terms of implementation details: the new _loc variant (I'm also 
finding useful a build_min_loc and a build_min_non_dep_loc, by the way) 
seems identical to the non-_loc variant besides calling 
SET_EXPR_LOCATION, thus I'm thinking: would it make sense to have the 
current non-_loc variants implemented in terms of the new ones and 
passing 0 as location_t and the _loc variant doing SET_EXPR_LOCATION (t, 
loc); only if loc != 0? Or we want to keep location_t opaque, I don't 
know, we want to do something slightly different?

Thanks,
Paolo.
Jason Merrill May 14, 2012, 4:24 p.m. UTC | #3 
On 05/14/2012 09:22 AM, Paolo Carlini wrote:
> In terms of implementation details: the new _loc variant (I'm also
> finding useful a build_min_loc and a build_min_non_dep_loc, by the way)
> seems identical to the non-_loc variant besides calling
> SET_EXPR_LOCATION, thus I'm thinking: would it make sense to have the
> current non-_loc variants implemented in terms of the new ones and

That's problematic with variadic functions.

Jason
Paolo Carlini May 14, 2012, 4:53 p.m. UTC | #4 
On 05/14/2012 06:24 PM, Jason Merrill wrote:
> On 05/14/2012 09:22 AM, Paolo Carlini wrote:
>> In terms of implementation details: the new _loc variant (I'm also
>> finding useful a build_min_loc and a build_min_non_dep_loc, by the way)
>> seems identical to the non-_loc variant besides calling
>> SET_EXPR_LOCATION, thus I'm thinking: would it make sense to have the
>> current non-_loc variants implemented in terms of the new ones and
> That's problematic with variadic functions.
Yeah. I'm trying to replace *all* the current ones with the _loc 
variant. The patch as you can imagine is becoming *huge*. Some details 
will probably need another pass.

Paolo.
Jason Merrill May 14, 2012, 5:38 p.m. UTC | #5 
On 05/14/2012 12:53 PM, Paolo Carlini wrote:
> Yeah. I'm trying to replace *all* the current ones with the _loc
> variant. The patch as you can imagine is becoming *huge*. Some details
> will probably need another pass.

One approach would be to make the non-_loc names macros that just call 
the _loc variants with UNKNOWN_LOCATION.

Jason
Paolo Carlini May 14, 2012, 5:40 p.m. UTC | #6 
On 05/14/2012 07:38 PM, Jason Merrill wrote:
> On 05/14/2012 12:53 PM, Paolo Carlini wrote:
>> Yeah. I'm trying to replace *all* the current ones with the _loc
>> variant. The patch as you can imagine is becoming *huge*. Some details
>> will probably need another pass.
> One approach would be to make the non-_loc names macros that just call 
> the _loc variants with UNKNOWN_LOCATION.
Ok, I'll do that, without trying in one swoop to replace *all* the 
non-loc ones, I'm drowning in the patch ;)

Paolo.
diff mbox

Patch

Index: testsuite/g++.dg/parse/error49.C
===================================================================
--- testsuite/g++.dg/parse/error49.C	(revision 0)
+++ testsuite/g++.dg/parse/error49.C	(revision 0)
@@ -0,0 +1,10 @@ 
+// PR c++/44516
+
+struct WebService {  };
+struct Server {  };
+
+void addHTTPService(Server const &server,
+		    WebService const *http)
+{
+  server += http; // { dg-error "10:no match for 'operator\\+='" }
+}
Index: cp/typeck.c
===================================================================
--- cp/typeck.c	(revision 187435)
+++ cp/typeck.c	(working copy)
@@ -7105,8 +7105,8 @@  cp_build_modify_expr (tree lhs, enum tree_code mod
 }
 
 tree
-build_x_modify_expr (tree lhs, enum tree_code modifycode, tree rhs,
-		     tsubst_flags_t complain)
+build_x_modify_expr (location_t loc, tree lhs, enum tree_code modifycode,
+		     tree rhs, tsubst_flags_t complain)
 {
   if (processing_template_decl)
     return build_min_nt (MODOP_EXPR, lhs,
@@ -7114,7 +7114,7 @@  tree
 
   if (modifycode != NOP_EXPR)
     {
-      tree rval = build_new_op (input_location, MODIFY_EXPR, LOOKUP_NORMAL,
+      tree rval = build_new_op (loc, MODIFY_EXPR, LOOKUP_NORMAL,
 				lhs, rhs, make_node (modifycode),
 				/*overload=*/NULL, complain);
       if (rval)
Index: cp/pt.c
===================================================================
--- cp/pt.c	(revision 187435)
+++ cp/pt.c	(working copy)
@@ -12672,7 +12672,8 @@  tsubst_omp_for_iterator (tree t, int i, tree declv
       cond = RECUR (TREE_VEC_ELT (OMP_FOR_COND (t), i));
       incr = TREE_VEC_ELT (OMP_FOR_INCR (t), i);
       if (TREE_CODE (incr) == MODIFY_EXPR)
-	incr = build_x_modify_expr (RECUR (TREE_OPERAND (incr, 0)), NOP_EXPR,
+	incr = build_x_modify_expr (EXPR_LOC_OR_HERE (incr),
+				    RECUR (TREE_OPERAND (incr, 0)), NOP_EXPR,
 				    RECUR (TREE_OPERAND (incr, 1)),
 				    complain);
       else
@@ -13692,7 +13693,8 @@  tsubst_copy_and_build (tree t,
     case MODOP_EXPR:
       {
 	tree r = build_x_modify_expr
-	  (RECUR (TREE_OPERAND (t, 0)),
+	  (EXPR_LOC_OR_HERE (TREE_OPERAND (t, 1)),
+	   RECUR (TREE_OPERAND (t, 0)),
 	   TREE_CODE (TREE_OPERAND (t, 1)),
 	   RECUR (TREE_OPERAND (t, 2)),
 	   complain);
Index: cp/semantics.c
===================================================================
--- cp/semantics.c	(revision 187435)
+++ cp/semantics.c	(working copy)
@@ -4456,7 +4456,8 @@  handle_omp_for_class_iterator (int i, location_t l
 	cond = error_mark_node;
       else
 	{
-	  tree tem = build_x_binary_op (input_location, TREE_CODE (cond),
+	  tree tem = build_x_binary_op (EXPR_LOC_OR_HERE (cond),
+					TREE_CODE (cond),
 					iter, ERROR_MARK,
 					TREE_OPERAND (cond, 1), ERROR_MARK,
 					NULL, tf_warning_or_error);
@@ -4473,7 +4474,7 @@  handle_omp_for_class_iterator (int i, location_t l
       error_at (elocus, "invalid controlling predicate");
       return true;
     }
-  diff = build_x_binary_op (input_location, MINUS_EXPR, TREE_OPERAND (cond, 1),
+  diff = build_x_binary_op (elocus, MINUS_EXPR, TREE_OPERAND (cond, 1),
 			    ERROR_MARK, iter, ERROR_MARK, NULL,
 			    tf_warning_or_error);
   if (error_operand_p (diff))
@@ -4496,7 +4497,8 @@  handle_omp_for_class_iterator (int i, location_t l
 	  incr = error_mark_node;
 	  break;
 	}
-      iter_incr = build_x_unary_op (input_location, TREE_CODE (incr), iter,
+      iter_incr = build_x_unary_op (EXPR_LOC_OR_HERE (incr),
+				    TREE_CODE (incr), iter,
 				    tf_warning_or_error);
       if (error_operand_p (iter_incr))
 	return true;
@@ -4520,7 +4522,8 @@  handle_omp_for_class_iterator (int i, location_t l
 		incr = error_mark_node;
 	      else
 		{
-		  iter_incr = build_x_modify_expr (iter, TREE_CODE (rhs),
+		  iter_incr = build_x_modify_expr (EXPR_LOC_OR_HERE (rhs),
+						   iter, TREE_CODE (rhs),
 						   TREE_OPERAND (rhs, 1),
 						   tf_warning_or_error);
 		  if (error_operand_p (iter_incr))
@@ -4546,14 +4549,16 @@  handle_omp_for_class_iterator (int i, location_t l
 		incr = error_mark_node;
 	      else
 		{
-		  iter_incr = build_x_binary_op (input_location, PLUS_EXPR,
+		  iter_incr = build_x_binary_op (EXPR_LOC_OR_HERE (rhs),
+						 PLUS_EXPR,
 						 TREE_OPERAND (rhs, 0),
 						 ERROR_MARK, iter,
 						 ERROR_MARK, NULL,
 						 tf_warning_or_error);
 		  if (error_operand_p (iter_incr))
 		    return true;
-		  iter_incr = build_x_modify_expr (iter, NOP_EXPR,
+		  iter_incr = build_x_modify_expr (EXPR_LOC_OR_HERE (rhs),
+						   iter, NOP_EXPR,
 						   iter_incr,
 						   tf_warning_or_error);
 		  if (error_operand_p (iter_incr))
@@ -4604,18 +4609,22 @@  handle_omp_for_class_iterator (int i, location_t l
   if (orig_pre_body)
     add_stmt (orig_pre_body);
   if (init != NULL)
-    finish_expr_stmt (build_x_modify_expr (iter, NOP_EXPR, init,
+    finish_expr_stmt (build_x_modify_expr (elocus,
+					   iter, NOP_EXPR, init,
 					   tf_warning_or_error));
   init = build_int_cst (TREE_TYPE (diff), 0);
   if (c && iter_incr == NULL)
     {
-      finish_expr_stmt (build_x_modify_expr (incr_var, NOP_EXPR,
+      finish_expr_stmt (build_x_modify_expr (elocus,
+					     incr_var, NOP_EXPR,
 					     incr, tf_warning_or_error));
       incr = incr_var;
-      iter_incr = build_x_modify_expr (iter, PLUS_EXPR, incr,
+      iter_incr = build_x_modify_expr (elocus,
+				       iter, PLUS_EXPR, incr,
 				       tf_warning_or_error);
     }
-  finish_expr_stmt (build_x_modify_expr (last, NOP_EXPR, init,
+  finish_expr_stmt (build_x_modify_expr (elocus,
+					 last, NOP_EXPR, init,
 					 tf_warning_or_error));
   *pre_body = pop_stmt_list (*pre_body);
 
@@ -4628,11 +4637,13 @@  handle_omp_for_class_iterator (int i, location_t l
   orig_body = *body;
   *body = push_stmt_list ();
   iter_init = build2 (MINUS_EXPR, TREE_TYPE (diff), decl, last);
-  iter_init = build_x_modify_expr (iter, PLUS_EXPR, iter_init,
+  iter_init = build_x_modify_expr (elocus,
+				   iter, PLUS_EXPR, iter_init,
 				   tf_warning_or_error);
   iter_init = build1 (NOP_EXPR, void_type_node, iter_init);
   finish_expr_stmt (iter_init);
-  finish_expr_stmt (build_x_modify_expr (last, NOP_EXPR, decl,
+  finish_expr_stmt (build_x_modify_expr (elocus,
+					 last, NOP_EXPR, decl,
 					 tf_warning_or_error));
   add_stmt (orig_body);
   *body = pop_stmt_list (*body);
Index: cp/parser.c
===================================================================
--- cp/parser.c	(revision 187435)
+++ cp/parser.c	(working copy)
@@ -7512,11 +7513,11 @@  cp_parser_assignment_expression (cp_parser* parser
 	return cp_parser_question_colon_clause (parser, expr);
       else
 	{
-	  enum tree_code assignment_operator;
+	  location_t loc = cp_lexer_peek_token (parser->lexer)->location;
 
 	  /* If it's an assignment-operator, we're using the second
 	     production.  */
-	  assignment_operator
+	  enum tree_code assignment_operator
 	    = cp_parser_assignment_operator_opt (parser);
 	  if (assignment_operator != ERROR_MARK)
 	    {
@@ -7534,7 +7535,7 @@  cp_parser_assignment_expression (cp_parser* parser
 							      NIC_ASSIGNMENT))
 		return error_mark_node;
 	      /* Build the assignment expression.  */
-	      expr = build_x_modify_expr (expr,
+	      expr = build_x_modify_expr (loc, expr,
 					  assignment_operator,
 					  rhs,
 					  tf_warning_or_error);
@@ -26350,7 +26356,8 @@  cp_parser_omp_for_loop (cp_parser *parser, tree cl
 		  cp_parser_parse_definitely (parser);
 		  cp_parser_require (parser, CPP_EQ, RT_EQ);
 		  rhs = cp_parser_assignment_expression (parser, false, NULL);
-		  finish_expr_stmt (build_x_modify_expr (decl, NOP_EXPR,
+		  finish_expr_stmt (build_x_modify_expr (EXPR_LOC_OR_HERE (rhs),
+							 decl, NOP_EXPR,
 							 rhs,
 							 tf_warning_or_error));
 		  add_private_clause = true;
Index: cp/cp-tree.h
===================================================================
--- cp/cp-tree.h	(revision 187435)
+++ cp/cp-tree.h	(working copy)
@@ -5835,7 +5835,8 @@  extern tree build_reinterpret_cast		(tree, tree, t
 extern tree build_const_cast			(tree, tree, tsubst_flags_t);
 extern tree build_c_cast			(location_t, tree, tree);
 extern tree cp_build_c_cast			(tree, tree, tsubst_flags_t);
-extern tree build_x_modify_expr			(tree, enum tree_code, tree,
+extern tree build_x_modify_expr			(location_t, tree,
+						 enum tree_code, tree,
 						 tsubst_flags_t);
 extern tree cp_build_modify_expr		(tree, enum tree_code, tree,
 						 tsubst_flags_t);