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

[Eli Friedman]

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.

> 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