[cfe-dev] Adding access to array size expressions in ConstantArrayType.

[Eli Friedman]

On Mon, Jun 15, 2009 at 5:42 AM, Enea Zaffanella<zaffanella at cs.unipr.it> wrote:
> Eli Friedman wrote:
>>
>> On Mon, Jun 15, 2009 at 1:32 AM, Enea Zaffanella<zaffanella at cs.unipr.it>
>> wrote:
>>>
>>> This Expr pointer will be 0 whenever the size expression is "canonical":
>>> namely, when the array size expression is just an integer literal
>>> expressed
>>> in decimal notation and having no suffix at all.
>>> On the one end, the client has no actual need to investigate such a
>>> canonical array size expression (since it is completely determined by the
>>> usual getSize); on the other hand, having a null pointer for this
>>> frequently
>>> occurring case will preserve the ability of making most of the
>>> occurrences
>>> of the same array type unique.
>>
>> It's hard to say whether this is the right tradeoff.  Do you have any
>> measurements?  Note that if this ends up being a significant expense,
>> we can always add an option to the ASTContext constructor (something
>> like "RichTypeInfo").
>>
>> I would strongly prefer that we either always or never include the
>> original expression; in addition to being more consistent, that would
>> allow getting at the location information for the original expression.
>
>
> Well, of course, we would be happy if that info is always available.
>
> The only reason we have proposed this solution is to avoid making the clang
> Type cache useless: if we put a non-null Expr* inside all ConstantArrayType
> objects, these types will always look different (since different occurrences
> of the same expression typically result in different Expr*). In other words,
> we did it this way in order to maximize our chances to see the patch
> accepted.
>
> If that is not an obstacle for getting the patch accepted, we'll be happy to
> modify the patch so as to have all ConstantArrayType objects contain the
> pointer to the size expression, no matter if the latter is canonical or not.

The point of the type cache isn't really to get hits for non-canonical
types (although it's nice when we do); the point is to make the
canonical types unique, which allows turning type equivalence into a
pointer comparison.

Again, the best thing here would be measurements of, for example,
building Python (or something simiilar): how many array types do we
create normally, how many get created with your scheme, and how many
get created keeping the expression for all array types?

> OK, we will remove the old constructor and add the missing null pointer to
> the other calls. Out of curiosity, since we went through *all* of the calls
> of the old constructor, which is the specific call of getConstantArrayType
> that you say we are mishandling?

Sorry, I didn't read the patch carefully enough; you did in fact catch
the case I was thinking of.

-Eli