[C++] Improve build_new locations

Hi,

some rather straightforward bits for build_new / 
cp_parser_new_expression, almost a clean-up. It would be nice to do 
better for the diagnostic issued from build_new_1 too but having 
consistent locations for all the valid_array_size_p and 
invalid_array_size_error calls isn't trivial, I'd rather work on that at 
a later time. Tested x86_64-linux.

Thanks, Paolo.

///////////////////////////
/gcc/cp
2020-01-02  Paolo Carlini  <paolo.carlini@oracle.com>

	* init.c (build_new): Add location_t parameter and use it throughout.
	(build_raw_new_expr): Likewise.
	* parser.c (cp_parser_new_expression): Pass the combined_loc.
	* pt.c (tsubst_copy_and_build): Adjust call.
	* cp-tree.h: Update declarations.

/libcc1
2020-01-02  Paolo Carlini  <paolo.carlini@oracle.com>

	* libcp1plugin.cc (plugin_build_new_expr): Update build_new call.

/gcc/testsuite
2020-01-02  Paolo Carlini  <paolo.carlini@oracle.com>

	* g++.old-deja/g++.bugs/900208_03.C: Check locations too.
	* g++.old-deja/g++.bugs/900519_06.C: Likewise.

On 1/2/20 4:23 AM, Paolo Carlini wrote:
> @@ -19320,8 +19320,8 @@ tsubst_copy_and_build (tree t,
>   
>   	tree op1 = tsubst (TREE_OPERAND (t, 1), args, complain, in_decl);
>   	tree op2 = RECUR (TREE_OPERAND (t, 2));
> -	ret = build_new (&placement_vec, op1, op2, &init_vec,
> -			 NEW_EXPR_USE_GLOBAL (t),
> +	ret = build_new (input_location, &placement_vec, op1, op2,

Hmm, using input_location bothers me even though it does have the right 
value in this function.  Let's use a local variable instead; maybe 
change the existing loc variable to save_loc and use the name loc for 
the location we want to use.

Jason

Hi,

On 06/01/20 21:47, Jason Merrill wrote:
> On 1/2/20 4:23 AM, Paolo Carlini wrote:
>> @@ -19320,8 +19320,8 @@ tsubst_copy_and_build (tree t,
>>         tree op1 = tsubst (TREE_OPERAND (t, 1), args, complain, 
>> in_decl);
>>       tree op2 = RECUR (TREE_OPERAND (t, 2));
>> -    ret = build_new (&placement_vec, op1, op2, &init_vec,
>> -             NEW_EXPR_USE_GLOBAL (t),
>> +    ret = build_new (input_location, &placement_vec, op1, op2,
>
> Hmm, using input_location bothers me even though it does have the 
> right value in this function.  Let's use a local variable instead; 
> maybe change the existing loc variable to save_loc and use the name 
> loc for the location we want to use.

I see, I wondered about that myself: when I started working on these 
location issues I forwarded simple EXPR_LOCATION (t) in a few places, 
which worked just fine. Then I noticed that in many existing places we 
were exploiting the fact that input_location is changed on top and 
started using it myself too. I suppose some of those places too could be 
changed to simple EXPR_LOCATION (t), I can experiment with that.

Anyway, the below implements the save_loc bit and regtests fine on 
x86_64-linux, as usual.

Thanks, Paolo.

/////////////////////////////
Index: gcc/cp/cp-tree.h
===================================================================
--- gcc/cp/cp-tree.h	(revision 279829)
+++ gcc/cp/cp-tree.h	(working copy)
@@ -6720,9 +6720,10 @@ extern tree throw_bad_array_new_length		(void);
 extern bool type_has_new_extended_alignment	(tree);
 extern unsigned malloc_alignment		(void);
 extern tree build_new_constexpr_heap_type	(tree, tree, tree);
-extern tree build_new				(vec<tree, va_gc> **, tree, tree,
-						 vec<tree, va_gc> **, int,
-                                                 tsubst_flags_t);
+extern tree build_new				(location_t,
+						 vec<tree, va_gc> **, tree,
+						 tree, vec<tree, va_gc> **,
+						 int, tsubst_flags_t);
 extern tree get_temp_regvar			(tree, tree);
 extern tree build_vec_init			(tree, tree, tree, bool, int,
                                                  tsubst_flags_t);
Index: gcc/cp/init.c
===================================================================
--- gcc/cp/init.c	(revision 279829)
+++ gcc/cp/init.c	(working copy)
@@ -2396,8 +2396,8 @@ decl_constant_value (tree decl)
    creates and returns a NEW_EXPR.  */
 
 static tree
-build_raw_new_expr (vec<tree, va_gc> *placement, tree type, tree nelts,
-		    vec<tree, va_gc> *init, int use_global_new)
+build_raw_new_expr (location_t loc, vec<tree, va_gc> *placement, tree type,
+		    tree nelts, vec<tree, va_gc> *init, int use_global_new)
 {
   tree init_list;
   tree new_expr;
@@ -2413,9 +2413,9 @@ static tree
   else
     init_list = build_tree_list_vec (init);
 
-  new_expr = build4 (NEW_EXPR, build_pointer_type (type),
-		     build_tree_list_vec (placement), type, nelts,
-		     init_list);
+  new_expr = build4_loc (loc, NEW_EXPR, build_pointer_type (type),
+			 build_tree_list_vec (placement), type, nelts,
+			 init_list);
   NEW_EXPR_USE_GLOBAL (new_expr) = use_global_new;
   TREE_SIDE_EFFECTS (new_expr) = 1;
 
@@ -3775,8 +3775,9 @@ build_new_1 (vec<tree, va_gc> **placement, tree ty
    rather than just "new".  This may change PLACEMENT and INIT.  */
 
 tree
-build_new (vec<tree, va_gc> **placement, tree type, tree nelts,
-	   vec<tree, va_gc> **init, int use_global_new, tsubst_flags_t complain)
+build_new (location_t loc, vec<tree, va_gc> **placement, tree type,
+	   tree nelts, vec<tree, va_gc> **init, int use_global_new,
+	   tsubst_flags_t complain)
 {
   tree rval;
   vec<tree, va_gc> *orig_placement = NULL;
@@ -3826,7 +3827,7 @@ tree
 	  || (nelts && type_dependent_expression_p (nelts))
 	  || (nelts && *init)
 	  || any_type_dependent_arguments_p (*init))
-	return build_raw_new_expr (*placement, type, nelts, *init,
+	return build_raw_new_expr (loc, *placement, type, nelts, *init,
 				   use_global_new);
 
       orig_placement = make_tree_vector_copy (*placement);
@@ -3852,10 +3853,11 @@ tree
 
   if (nelts)
     {
+      location_t nelts_loc = cp_expr_loc_or_loc (nelts, loc);
       if (!build_expr_type_conversion (WANT_INT | WANT_ENUM, nelts, false))
         {
           if (complain & tf_error)
-	    permerror (cp_expr_loc_or_input_loc (nelts),
+	    permerror (nelts_loc,
 		       "size in array new must have integral type");
           else
             return error_mark_node;
@@ -3871,8 +3873,7 @@ tree
 	 less than zero. ... If the expression is a constant expression,
 	 the program is ill-fomed.  */
       if (TREE_CODE (cst_nelts) == INTEGER_CST
-	  && !valid_array_size_p (cp_expr_loc_or_input_loc (nelts),
-				  cst_nelts, NULL_TREE,
+	  && !valid_array_size_p (nelts_loc, cst_nelts, NULL_TREE,
 				  complain & tf_error))
 	return error_mark_node;
 
@@ -3886,7 +3887,7 @@ tree
   if (TYPE_REF_P (type))
     {
       if (complain & tf_error)
-        error ("new cannot be applied to a reference type");
+        error_at (loc, "new cannot be applied to a reference type");
       else
         return error_mark_node;
       type = TREE_TYPE (type);
@@ -3895,7 +3896,7 @@ tree
   if (TREE_CODE (type) == FUNCTION_TYPE)
     {
       if (complain & tf_error)
-        error ("new cannot be applied to a function type");
+        error_at (loc, "new cannot be applied to a function type");
       return error_mark_node;
     }
 
@@ -3911,7 +3912,7 @@ tree
 
   if (processing_template_decl)
     {
-      tree ret = build_raw_new_expr (orig_placement, type, orig_nelts,
+      tree ret = build_raw_new_expr (loc, orig_placement, type, orig_nelts,
 				     orig_init, use_global_new);
       release_tree_vector (orig_placement);
       release_tree_vector (orig_init);
@@ -3919,7 +3920,7 @@ tree
     }
 
   /* Wrap it in a NOP_EXPR so warn_if_unused_value doesn't complain.  */
-  rval = build1 (NOP_EXPR, TREE_TYPE (rval), rval);
+  rval = build1_loc (loc, NOP_EXPR, TREE_TYPE (rval), rval);
   TREE_NO_WARNING (rval) = 1;
 
   return rval;
Index: gcc/cp/parser.c
===================================================================
--- gcc/cp/parser.c	(revision 279829)
+++ gcc/cp/parser.c	(working copy)
@@ -8750,11 +8750,9 @@ cp_parser_new_expression (cp_parser* parser)
          at the end of the final token we consumed.  */
       location_t combined_loc = make_location (start_loc, start_loc,
 					       parser->lexer);
-
       /* Create a representation of the new-expression.  */
-      ret = build_new (&placement, type, nelts, &initializer, global_scope_p,
-		       tf_warning_or_error);
-      protected_set_expr_location (ret, combined_loc);
+      ret = build_new (combined_loc, &placement, type, nelts, &initializer,
+		       global_scope_p, tf_warning_or_error);
     }
 
   if (placement != NULL)
Index: gcc/cp/pt.c
===================================================================
--- gcc/cp/pt.c	(revision 279829)
+++ gcc/cp/pt.c	(working copy)
@@ -18820,12 +18820,12 @@ tsubst_copy_and_build (tree t,
 			 integral_constant_expression_p)
 
   tree retval, op1;
-  location_t loc;
+  location_t save_loc;
 
   if (t == NULL_TREE || t == error_mark_node)
     return t;
 
-  loc = input_location;
+  save_loc = input_location;
   if (location_t eloc = cp_expr_location (t))
     input_location = eloc;
 
@@ -19285,6 +19285,7 @@ tsubst_copy_and_build (tree t,
 	vec<tree, va_gc> *placement_vec;
 	vec<tree, va_gc> *init_vec;
 	tree ret;
+	location_t loc = EXPR_LOCATION (t);
 
 	if (placement == NULL_TREE)
 	  placement_vec = NULL;
@@ -19320,8 +19321,8 @@ tsubst_copy_and_build (tree t,
 
 	tree op1 = tsubst (TREE_OPERAND (t, 1), args, complain, in_decl);
 	tree op2 = RECUR (TREE_OPERAND (t, 2));
-	ret = build_new (&placement_vec, op1, op2, &init_vec,
-			 NEW_EXPR_USE_GLOBAL (t),
+	ret = build_new (loc, &placement_vec, op1, op2,
+			 &init_vec, NEW_EXPR_USE_GLOBAL (t),
 			 complain);
 
 	if (placement_vec != NULL)
@@ -20231,7 +20232,7 @@ tsubst_copy_and_build (tree t,
 #undef RECUR
 #undef RETURN
  out:
-  input_location = loc;
+  input_location = save_loc;
   return retval;
 }
 
Index: gcc/testsuite/g++.old-deja/g++.bugs/900208_03.C
===================================================================
--- gcc/testsuite/g++.old-deja/g++.bugs/900208_03.C	(revision 279829)
+++ gcc/testsuite/g++.old-deja/g++.bugs/900208_03.C	(working copy)
@@ -13,7 +13,7 @@ typedef void (func_type) ();
 
 void global_function_0 ()
 {
-  new func_type;	// { dg-error "" } missed by both cfront 2.0 and g++ 1.36.1
+  new func_type;	// { dg-error "3:new cannot be applied to a function type" } missed by both cfront 2.0 and g++ 1.36.1
 }
 
 int main () { return 0; }
Index: gcc/testsuite/g++.old-deja/g++.bugs/900519_06.C
===================================================================
--- gcc/testsuite/g++.old-deja/g++.bugs/900519_06.C	(revision 279829)
+++ gcc/testsuite/g++.old-deja/g++.bugs/900519_06.C	(working copy)
@@ -12,12 +12,12 @@ typedef int& int_ref;
 
 void test (int n)
 {
-  new int&;		// { dg-error "" } missed
-  new int_ref;		// { dg-error "" } missed
+  new int&;		// { dg-error "3:new cannot be applied to a reference type" } missed
+  new int_ref;		// { dg-error "3:new cannot be applied to a reference type" } missed
   new int&[n];		// { dg-error "" } missed
-  new int_ref[n];	// { dg-error "" } missed
+  new int_ref[n];	// { dg-error "3:new cannot be applied to a reference type" } missed
   new int&[3];		// { dg-error "" } missed
-  new int_ref[3];	// { dg-error "" } missed
+  new int_ref[3];	// { dg-error "3:new cannot be applied to a reference type" } missed
 }
 
 int main () { return 0; }
Index: libcc1/libcp1plugin.cc
===================================================================
--- libcc1/libcp1plugin.cc	(revision 279829)
+++ libcc1/libcp1plugin.cc	(working copy)
@@ -3258,8 +3258,8 @@ plugin_build_new_expr (cc1_plugin::connection *sel
   if (!template_dependent_p)
     processing_template_decl--;
 
-  tree result = build_new (&placement, type, nelts, &initializer,
-			   global_scope_p, tf_error);
+  tree result = build_new (input_location, &placement, type, nelts,
+			   &initializer, global_scope_p, tf_error);
 
   if (template_dependent_p)
     processing_template_decl--;

On 1/7/20 5:46 AM, Paolo Carlini wrote:
> Hi,
> 
> On 06/01/20 21:47, Jason Merrill wrote:
>> On 1/2/20 4:23 AM, Paolo Carlini wrote:
>>> @@ -19320,8 +19320,8 @@ tsubst_copy_and_build (tree t,
>>>         tree op1 = tsubst (TREE_OPERAND (t, 1), args, complain, 
>>> in_decl);
>>>       tree op2 = RECUR (TREE_OPERAND (t, 2));
>>> -    ret = build_new (&placement_vec, op1, op2, &init_vec,
>>> -             NEW_EXPR_USE_GLOBAL (t),
>>> +    ret = build_new (input_location, &placement_vec, op1, op2,
>>
>> Hmm, using input_location bothers me even though it does have the 
>> right value in this function.  Let's use a local variable instead; 
>> maybe change the existing loc variable to save_loc and use the name 
>> loc for the location we want to use.
> 
> I see, I wondered about that myself: when I started working on these 
> location issues I forwarded simple EXPR_LOCATION (t) in a few places, 
> which worked just fine. Then I noticed that in many existing places we 
> were exploiting the fact that input_location is changed on top and 
> started using it myself too. I suppose some of those places too could be 
> changed to simple EXPR_LOCATION (t), I can experiment with that.
> 
> Anyway, the below implements the save_loc bit and regtests fine on 
> x86_64-linux, as usual.

OK.