Re: [PATCH/RFC] C++ FE: expression ranges (v2)

[Jason Merrill <jason@redhat.com>]

On 11/19/2015 03:46 PM, Jason Merrill wrote:
> On 11/15/2015 12:01 AM, David Malcolm wrote:
>> As with the C frontend, there's an issue with tree nodes that
>> don't have locations: VAR_DECL, INTEGER_CST, etc:
>>
>>    int test (int foo)
>>    {
>>      return foo * 100;
>>             ^^^   ^^^
>>    }
>>
>> where we'd like to access the source spelling ranges of the expressions
>> during parsing, so that we can use them when reporting parser errors.
>
> Hmm, I had been thinking to address this in the C++ front end by
> wrapping uses in another tree: NOP_EXPR for rvalues, VIEW_CONVERT_EXPR
> for lvalues.

On the other hand, my direction seems likely to cause more issues, 
especially with code that doesn't yet know how to handle 
VIEW_CONVERT_EXPR, and could create ambiguity with explicit conversions. 
  So I guess your approach seems reasonable.

What is the memory consumption impact of this change?

> Also, in cp_parser_new_expression I attempted to generate meaningful
> ranges e.g.:
>
>   int *foo = new int[100];
>              ^~~~~~~~~~~~
>
> but it seems to be hard to do this, due to the trailing optional
> components; I found myself wanting to ask the lexer for the last
> token it consumed (in particular, I'm interested in the
> location_t of that token).  Is this a reasonable thing to add to the
> lexer?

cp_lexer_previous_token seems like what you want.

> -      return cp_build_unary_op (code, arg1, candidates != 0, complain);
> +      {
> +	tree result = cp_build_unary_op (code, arg1, candidates != 0, complain);
> +	protected_set_expr_location (result, loc);

I'd much rather pass loc into cp_build_unary_op.  At least add a FIXME 
to that effect.  Likewise in the other places you call *build* and then 
set the loc.

> +#if 0
> +    /* FIXME: various assertions can be put in here when debugging,
> +       for tracking down where location information gets thrown
> +       away (during a trip through a purely "tree" value).  */
> +    if (m_value && m_value != error_mark_node)
> +      {
> +	if (TREE_CODE (m_value) == FUNCTION_DECL)
> +	  return; // for now
> +	gcc_assert (CAN_HAVE_LOCATION_P (m_value));
> +	//gcc_assert (m_loc != UNKNOWN_LOCATION);

Yeah, I don't think you want to assert on UNKNOWN_LOCATION; some code 
does not and should not have an associated location.  In particular, 
cleanups.

>  Build the assignment expression.  Its default
> -		 location is the location of the '=' token.  */
> +		 location:
> +		   LHS = RHS
> +		   ~~~~^~~~~
> +		 is the location of the '=' token as the
> +		 caret, ranging from the start of the lhs to the
> +		 end of the rhs.  */
>  	      saved_input_location = input_location;
> +	      loc = make_location (loc,
> +				   expr.get_start (),
> +				   rhs.get_finish ());
>  	      input_location = loc;
>  	      expr = build_x_modify_expr (loc, expr,
>  					  assignment_operator,
>  					  rhs,
>  					  complain_flags (decltype_p));
> +	      protected_set_expr_location (expr, loc);
>  	      input_location = saved_input_location;

Do we still need to mess with input_location here?  If so, please add a 
FIXME explaining what still needs to be fixed.

Jason