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

[Enea Zaffanella]

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.

> 
>> Note also that we haven't modified the clang AST pretty printer to show this
>> info instead of the computed size, because we don't know whether or not this
>> is the desired behaviour.
> 
> It probably depends on the context; when we're in -ast-print mode,
> we'll definitely want the original expression, but we probably don't
> want to be pretty-printing expressions inside diagnostics.  Can you
> add it as an option to PrintingPolicy?
> 
>> As a side note, we observed that the array size in ConstantArrayType is one
>> of the few places where the original syntax of the program was replaced and
>> made no longer available in the AST. In our application we generally need to
>> be able to recover the original syntax (for instance, we appreciated very
>> much the existence of class OriginalParmVarDecl). So, if you know of other
>> places where such a replacement is performed, please let us know ... and
>> consider if it would be possible to avoid it ... that would be very helpful
>> for our kind of clients.
> 
> Off the top of my head, I can only think of a few things.  One, there
> are a whole bunch of issues related to attributes: certain attributes
> aren't preserved in the AST (like vector_size, and unknown
> attributes), we don't preserve expressions in attributes, we don't
> preserve any location information for attributes, and we don't
> preserve whether an attribute applies to a set of declarations or an
> individual declaration.  Two, we don't properly track the definition
> of the struct in an statement like "return (struct {int x;}){1}.x;"
> (the definition itself is available, but it's nearly impossible to
> figure out which expression defines the struct).  Three, we don't keep
> track of whether declarations are part of the same declaration group
> for global/field declarations like "int x,y;".
> 
>> If this patch will be accepted, we'll follow with another patch to
>> discriminate between the case when OrigSizeExpr == 0 due to the expression
>> being canonical, with respect to the case when OrigSizeExpr == 0 due to a
>> missing size expression that gets filled in in Sema (e.g., int a[] =  {2,3})
>> (see http://lists.cs.uiuc.edu/pipermail/cfe-dev/2009-February/004426.html).
> 
> Okay, that sounds good.
> 
> A few review comments below.
> 
>> Index: include/clang/AST/ASTContext.h
>> ===================================================================
>> --- include/clang/AST/ASTContext.h      (revision 73189)
>> +++ include/clang/AST/ASTContext.h      (working copy)
>> @@ -275,6 +275,16 @@
>>   /// constant array of the specified element type.
>>   QualType getConstantArrayType(QualType EltTy, const llvm::APInt &ArySize,
>>                                 ArrayType::ArraySizeModifier ASM,
>> +                                unsigned EltTypeQuals) {
>> +    // Pass in a null pointer for the original array size expression.
>> +    return getConstantArrayType(EltTy, ArySize, 0, ASM, EltTypeQuals);
>> +  }
>> +
>> +  /// getConstantArrayType - Return the unique reference to the type for a
>> +  /// constant array of the specified element type.
>> +  QualType getConstantArrayType(QualType EltTy, const llvm::APInt &ArySize,
>> +                                Expr *Original_ArySize,
>> +                                ArrayType::ArraySizeModifier ASM,
>>                                 unsigned EltTypeQuals);
> 
> I would prefer that the expr is always passed in explicitly; in
> addition to getting rid of two confusingly similar prototypes, it'll
> make it easier to audit the various callers in the patch.  (There's at
> least one caller which I know isn't doing the right thing here.)
> 
> -Eli

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?

Cheers,
Abramo Bagnara and Enea Zaffanella.


> 
>