[prev in list] [next in list] [prev in thread] [next in thread] 

List:       gcc-patches
Subject:    Re: [PATCH 4/5] Implement tree expression tracking in C FE (v2)
From:       Dodji Seketeli <dodji () seketeli ! org>
Date:       2015-09-25 16:15:01
Message-ID: 86fv22lblm.fsf () seketeli ! org
[Download RAW message or body]

David Malcolm <dmalcolm@redhat.com> a écrit:

> On Fri, 2015-09-25 at 16:06 +0200, Dodji Seketeli wrote:
>> Hello,
>> 
>> Similarly to a comment I made on the previous patch of the series,
>> 
>> +++ b/libcpp/include/line-map.h
>> 
>> [...]
>> 
>>      struct GTY(()) location_adhoc_data {
>>        source_location locus;
>>     +  source_range src_range;
>>        void * GTY((skip)) data;
>>      };
>> 
>> Could we just change the type of locus and make it be a source_range
>> instead?  With the right converting constructor (in the source_range
>> class) that takes a source_location, the amount of churn should
>> hopefully be minimized, or maybe I am missing something ...
>
> Thanks.
>
> I think that the above is one place where we *would* want both locus and
> src_range.

Right, as opposed to the token case where conceptually, all we need is
the begining and the end of the token.  My confusion.

[...]

> As noted elsewhere, we might try to pack short ranges directly into the
> 32 bits of source_location:
>  https://gcc.gnu.org/ml/gcc-patches/2015-09/msg01826.html
> which would avoid having to use ad-hoc for such short ranges; ideally
> most tokens.  I'm experimenting with that (I don't have it fully working
> yet).

Right.  My personal inclination would be to make the general case of
storing all ranges in this adhoc data structure, or, even, into another
on-the-side data structure, similar to the adhoc one, but dedicated to
range associated to points, as you see fit.

Then when that works, consider the optimization of stuffing short ranges
into the 32 bits of source_location, depending on the memory profiles we
get.  But it's your call :-)

[...]

>> +location_t
>> +set_block (location_t loc, tree block)
>> +{
>> 
>> Likewise.  I am wondering if we shouldn't even change the name of this
>> function to better suit what it does: associate a tree to a location.
>
> "associate_tree_with_location" ?

If you find it cool, I am cool :-)

[...]

>> +++ b/gcc/c/c-tree.h
>> @@ -132,6 +132,9 @@ struct c_expr
>>       The type of an enum constant is a plain integer type, but this
>>       field will be the enum type.  */
>>    tree original_type;
>> +
>> +  /* FIXME.  */
>> +  source_range src_range;
>>  };
>> 
>> Why a FIXME here?
>
> To remind myself before posting the patches that I need to give the
> field a descriptive comment, explaining what purpose it serves.
>
> Oops :)
>
> It probably should read something like this:
>
>   /* The source range of this C expression.  This might
>      be thought of as redundant, since ranges for
>      expressions can be stored in a location_t, but
>      not all tree nodes in expressions have a location_t.
>
>      Consider this source code:  
>
> 	int test (int foo)
> 	{
> 	  return foo * 100;
>                 ^^^   ^^^
>        }
>
>     During C parsing, the ranges for "foo" and "100" are
>     stored within this field of c_expr, but after parsing
>     to GENERIC, all we have is a VAR_DECL and an
>     INTEGER_CST (the former's location is in at the top of the
>     function, and the latter has no location).
>
>     For consistency, we store ranges for all expressions
>     in this field, not just those that don't have a
>     location_t. */
>   source_range src_range;

Great, thanks.

[...]

> [BTW, I'm about to disappear on a vacation from tomorrow until October
> 6th, and will be away from email and computers]

Thanks for the heads-up.

Cheers,

-- 
		Dodji
[prev in list] [next in list] [prev in thread] [next in thread]