Adding access to array size expressions in ConstantArrayType.

Clang Frontend
#1

Dear cfe-list,

the attached patch is a proposal for a minor change in class ConstantArrayType.

The change is motivated by clients that need to be able to investigate the syntactic structure of the array size expression, for instance, to keep track of the use of magic constant numbers. This is not possible with the current AST structure (unless one operates before Sema ...).

To this end, we propose the addition of a new private data member to the class ConstantArrayType

     Expr* OrigSizeExpr;

_optionally_ referring to the "original non-canonical array size expression". The value of the Expr* can be queried by clients using public method

   Expr* ConstantArrayType::getOriginalSizeExpr() const;

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.

This comes at the cost to lose the ability to reach the original array size expression and its program points if the array size is canonical, but we thought it was a reasonable tradeoff; please give some feedback if you would prefer otherwise.

Hence, a constant array type will be canonical iff its element type is canonical _and_ its array size expression is canonical (i.e., the Expr* is null). For example, int[4] will be canonical and different from int[sizeof(int)], int[2+2], and int[0x04], which are all non-canonical.

Please, let us know if we are missing something important.

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.

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.

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).

Cheers,
Abramo Bagnara and Enea Zaffanella.

ConstantArrayType.diff (10.7 KB)

#2

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

#3

Eli Friedman 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.

#4

Enea Zaffanella wrote:

Eli Friedman wrote:

[...]

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.

Here is the alternative version of the patch where the expression pointer is always passed explicitly. Please, let us know of this is ok.

Cheers,
Enea Zaffanella

ConstantArrayType-ExplicitConstructor.diff (16.4 KB)

#5

Eli Friedman 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

#6

Eli Friedman wrote:

[...]

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?

We opted for a "grep-like" pass on Python ... this means we will just provide a rough idea ... but quite informative, given that we are talking about ~16MB of code.

Overall, there are 1072 array type declarations. Of these, 588 have a decimal unsuffixed integer literal as size expression; some other 72 have a single macro name as size expression, which might be expanded to such a "canonical" integer constant.

All considered, you are basically right in saying that the optimization is not worth the effort. So, if you say it is OK, we will proceed as you suggested and add the original size expression pointer unconditionally.

Abramo and Enea.

#7

That makes sense to me. One other request: please make the non-canonical ConstantArrayType and the canonical ConstantArrayType have a different class name. The canonical one should be our current ConstantArrayType that has an APInt for the size. I'd recommend naming the new one ConstantArrayWithExprType or something like that. While you're at it, you can put SourceLocation's for the [ and ] in there too :slight_smile:

-Chris

#8

Chris Lattner wrote:

Overall, there are 1072 array type declarations. Of these, 588 have a
decimal unsuffixed integer literal as size expression; some other 72
have a single macro name as size expression, which might be expanded to
such a "canonical" integer constant.

All considered, you are basically right in saying that the optimization
is not worth the effort. So, if you say it is OK, we will proceed as you
suggested and add the original size expression pointer unconditionally.

That makes sense to me. One other request: please make the non-canonical ConstantArrayType and the canonical ConstantArrayType have a different class name. The canonical one should be our current ConstantArrayType that has an APInt for the size. I'd recommend naming the new one ConstantArrayWithExprType or something like that. While you're at it, you can put SourceLocation's for the [ and ] in there too :slight_smile:

-Chris

Here we are with the patch.

We have added _two_ new classes deriving from ConstantArraySize:
   - ConstantArraySizeWithExpr
   - ConstantArraySizeWithoutExpr

The latter is for constructs such as
   int a = { 0, 1, 2, 3 ];
   char s = "A string";
that the parser initially builds as IncompleteArrayType objects and Sema later transforms to ConstantArrayWithoutExpr objects.

Regarding pretty-printing, we have added a flag in PrintingPolicy (ConstantArraySizeAsWritten, defaulting to false) that guides methods getAsStringInternal.

We have an issue we do not know how to solve, but we believe should be routine for someone knowing more about clang internals (there are a couple of FIXME in the patch). The problem is in SemaDecl.cpp:

static QualType
TryToFixInvalidVariablyModifiedType(QualType T,
                                     ASTContext &Context,
                                     bool &SizeIsNegative)

Here we need to transform a VariableArray into a ConstantArrayWithExpr. To avoid resource ownership issues, we should either steal or clone the VLA size expression and put it in the ConstantArrayWithExpr.
Since we do not know how to do it, in the proposed patch we simply copy the SizeExpr pointer as is ... leading to a shared resource. To avoid the double-deletion problem, we do *not* delete it in the destructor of ConstantArrayWithExpr.

The patch passes all of the clang tests
(in debugging mode; 14 expected failures).

Regarding the addition of SourceLocation's for the array brackets: we haven't implemented it (yet). I guess that, in order to do it, we should change something in the parsing process (i.e., before the AST build process) so as to gather and propagate the required info.
Could you please provide us with a hint regarding the portions of code we should modify?

Cheers,
Enea.

ConstantArrayWithExpr.patch (20.8 KB)

#9

We have an issue we do not know how to solve, but we believe should be
routine for someone knowing more about clang internals (there are a couple
of FIXME in the patch). The problem is in SemaDecl.cpp:

static QualType
TryToFixInvalidVariablyModifiedType(QualType T,
ASTContext &Context,
bool &SizeIsNegative)

Here we need to transform a VariableArray into a ConstantArrayWithExpr. To
avoid resource ownership issues, we should either steal or clone the VLA
size expression and put it in the ConstantArrayWithExpr.
Since we do not know how to do it, in the proposed patch we simply copy the
SizeExpr pointer as is ... leading to a shared resource. To avoid the
double-deletion problem, we do *not* delete it in the destructor of
ConstantArrayWithExpr.

The ownership is already kind of messy here; it's okay to leave it
like that for now. Eventually, we want the parent Decl to destroy it,
I think.

Regarding the addition of SourceLocation's for the array brackets: we
haven't implemented it (yet). I guess that, in order to do it, we should
change something in the parsing process (i.e., before the AST build process)
so as to gather and propagate the required info.
Could you please provide us with a hint regarding the portions of code we
should modify?

getSourceRange on a Declarator has the right locations, I think.

-Eli

#10

Eli Friedman wrote:

We have an issue we do not know how to solve, but we believe should be
routine for someone knowing more about clang internals (there are a couple
of FIXME in the patch). The problem is in SemaDecl.cpp:

static QualType
TryToFixInvalidVariablyModifiedType(QualType T,
                                   ASTContext &Context,
                                   bool &SizeIsNegative)

Here we need to transform a VariableArray into a ConstantArrayWithExpr. To
avoid resource ownership issues, we should either steal or clone the VLA
size expression and put it in the ConstantArrayWithExpr.
Since we do not know how to do it, in the proposed patch we simply copy the
SizeExpr pointer as is ... leading to a shared resource. To avoid the
double-deletion problem, we do *not* delete it in the destructor of
ConstantArrayWithExpr.

The ownership is already kind of messy here; it's okay to leave it
like that for now. Eventually, we want the parent Decl to destroy it,
I think.

Regarding the addition of SourceLocation's for the array brackets: we
haven't implemented it (yet). I guess that, in order to do it, we should
change something in the parsing process (i.e., before the AST build process)
so as to gather and propagate the required info.
Could you please provide us with a hint regarding the portions of code we
should modify?

getSourceRange on a Declarator has the right locations, I think.

-Eli

Hello.

Here is the completed patch, where the array type classes have a new method

   SourceRange getBracketsRange() const;

returning the source range going from the left to the right bracket tokens. Additional helper methods have been added to get just one of the two bracket locations:

   SourceLocation getLBracketLoc() const { return Brackets.getBegin(); }
   SourceLocation getRBracketLoc() const { return Brackets.getEnd(); }

According to the tests we made, everything is working as expected.
We hope to see the patch applied soon.

Cheers,
Enea Zaffanella.

ConstantArrayWithExprAndBrackets.patch (43.5 KB)

#11

While at it, we found a transient bug in revision 74387
which is due to a double free of an Expr pointer.

The testcase showing the bug, which has been extracted from test/SemaTemplate/temp_class_spec_neg.cpp, is as follows:

#12

@@ -816,6 +816,11 @@
/// 'int X[static restrict 4]'. For function parameters only.
unsigned IndexTypeQuals : 3;

+ /// LBLoc - The location of the left bracket.
+ SourceLocation LBLoc;
+ /// RBLoc - The location of the right bracket.
+ SourceLocation RBLoc;
+

These members appear to be unused.

Index: include/clang/Parse/DeclSpec.h

--- include/clang/Parse/DeclSpec.h (revision 74387)
+++ include/clang/Parse/DeclSpec.h (working copy)
@@ -488,11 +488,16 @@
/// True if this dimension was [*]. In this case, NumElts is null.
bool isStar : 1;

+ /// The location of the left and right brackets.
+ SourceLocation* RBracketLoc;
+
/// This is the size of the array, or null if [] or [*] was specified.
/// Since the parser is multi-purpose, and we don't want to impose a
root
/// expression class on all clients, NumElts is untyped.
ActionBase::ExprTy *NumElts;
- void destroy() {}
+ void destroy() {
+ delete RBracketLoc;
+ }
};

/// ParamInfo - An array of paraminfo objects is allocated whenever a
function
@@ -696,13 +701,14 @@
///
static DeclaratorChunk getArray(unsigned TypeQuals, bool isStatic,
bool isStar, void *NumElts,
- SourceLocation Loc) {
+ SourceLocation LBLoc, SourceLocation
RBLoc) {
DeclaratorChunk I;
I.Kind = Array;
- I.Loc = Loc;
+ I.Loc = LBLoc;
I.Arr.TypeQuals = TypeQuals;
I.Arr.hasStatic = isStatic;
I.Arr.isStar = isStar;
+ I.Arr.RBracketLoc = new SourceLocation(RBLoc);
I.Arr.NumElts = NumElts;
return I;
}

Don't allocate a single SourceLocation on the heap; SourceLocations
are really small (less than or equal to the size of a pointer), so
this doesn't actually save any space. Also, I'd just name the member
EndLoc; having it around could also be useful for function
declarators, and perhaps a couple of other kinds of declarators.

Index: lib/Sema/SemaTemplateInstantiate.cpp

--- lib/Sema/SemaTemplateInstantiate.cpp (revision 74387)
+++ lib/Sema/SemaTemplateInstantiate.cpp (working copy)
@@ -382,9 +382,23 @@
IntegerLiteral ArraySize(Size, SizeType, Loc);
return SemaRef.BuildArrayType(ElementType, T->getSizeModifier(),
&ArraySize, T->getIndexTypeQualifier(),
- Loc, Entity);
+ SourceRange(), Entity);
}

It would be nice to have a proper source range here; can you at least
put in a FIXME?

+QualType
+TemplateTypeInstantiator::InstantiateConstantArrayWithExprType
+(const ConstantArrayWithExprType *T) const {
+ const ConstantArrayType *CAT = T;
+ return InstantiateConstantArrayType(CAT);
+}
+
+QualType
+TemplateTypeInstantiator::InstantiateConstantArrayWithoutExprType
+(const ConstantArrayWithoutExprType *T) const {
+ const ConstantArrayType *CAT = T;
+ return InstantiateConstantArrayType(CAT);
+}

There's no point to the extra variables; just pass in T directly to
InstantiateConstantArrayType.

QualType
TemplateTypeInstantiator::
InstantiateIncompleteArrayType(const IncompleteArrayType *T) const {
@@ -393,8 +407,8 @@
return ElementType;

return SemaRef.BuildArrayType(ElementType, T->getSizeModifier(),
- 0, T->getIndexTypeQualifier(),
- Loc, Entity);
+ 0, T->getIndexTypeQualifier(),
+ SourceRange(), Entity);
}

Please add a FIXME for the missing source range.

QualType
@@ -429,7 +443,8 @@

return SemaRef.BuildArrayType(ElementType, T->getSizeModifier(),
InstantiatedArraySize.takeAs<Expr>(),
- T->getIndexTypeQualifier(), Loc, Entity);
+ T->getIndexTypeQualifier(),
+ SourceRange(), Entity);
}

Please add a FIXME for the missing source range.

Index: lib/AST/ASTContext.cpp

--- lib/AST/ASTContext.cpp (revision 74387)
+++ lib/AST/ASTContext.cpp (working copy)
@@ -274,6 +274,10 @@
Align = getTypeAlign(cast<ArrayType>(T)->getElementType());
break;

+ case Type::ConstantArrayWithExpr:
+ case Type::ConstantArrayWithoutExpr:
+ T = T->getCanonicalTypeInternal().getTypePtr();
+ // Intentionally falling through next case.

Why do you need to call getCanonicalTypeInternal here? Why can't they
just share the case with ConstantArray?

@@ -1184,7 +1245,8 @@

QualType ASTContext::getIncompleteArrayType(QualType EltTy,
ArrayType::ArraySizeModifier
ASM,
- unsigned EltTypeQuals) {
+ unsigned EltTypeQuals,
+ SourceRange Brackets) {
llvm::FoldingSetNodeID ID;
IncompleteArrayType::Profile(ID, EltTy, ASM, EltTypeQuals);

@@ -1199,7 +1261,7 @@

if (!EltTy->isCanonical()) {
Canonical = getIncompleteArrayType(getCanonicalType(EltTy),
- ASM, EltTypeQuals);
+ ASM, EltTypeQuals, Brackets);

// Get the new insert position for the node we care about\.
IncompleteArrayType \*NewIP =

@@ -1207,8 +1269,9 @@
assert(NewIP == 0 && "Shouldn't be in the map!"); NewIP = NewIP;
}

- IncompleteArrayType *New = new (*this,8) IncompleteArrayType(EltTy,
Canonical,
- ASM,
EltTypeQuals);
+ IncompleteArrayType *New
+ = new (*this,8) IncompleteArrayType(EltTy, Canonical,
+ ASM, EltTypeQuals, Brackets);

IncompleteArrayTypes.InsertNode(New, InsertPos);
Types.push_back(New);

Unlike the other array types you're adding Brackets to,
IncompleteArrayType gets canonicalized; we don't want a source range
on a canonical IncompleteArrayType, and we don't want to end up with
the wrong SourceRange on a non-canonical IncompleteArrayType. It's a
little tricky, so if the issues I'm talking about aren't obvious, I'd
suggest skipping adding the bracket locations to IncompleteArrayType
for now.

Otherwise, this is looking good!

-Eli

#13

Eli Friedman wrote:

[...]

Unlike the other array types you're adding Brackets to,
IncompleteArrayType gets canonicalized; we don't want a source range
on a canonical IncompleteArrayType, and we don't want to end up with
the wrong SourceRange on a non-canonical IncompleteArrayType. It's a
little tricky, so if the issues I'm talking about aren't obvious, I'd
suggest skipping adding the bracket locations to IncompleteArrayType
for now.

Otherwise, this is looking good!

-Eli

OK, here is the patch revised along your suggestions.

Enea.

ConstantArrayWithExprAndBrackets-Revised.patch (39.1 KB)

#14

This is definitely a bug; there are a bunch of "Clone this expression!" FIXMEs within the template argument-handling code, where we end up sharing expressions then trying to delete them later. I've filed this as http://llvm.org/bugs/show_bug.cgi?id=4488 and will fix it soon.

  - Doug

#15

Enea Zaffanella wrote:

Eli Friedman wrote:

[...]

Unlike the other array types you're adding Brackets to,
IncompleteArrayType gets canonicalized; we don't want a source range
on a canonical IncompleteArrayType, and we don't want to end up with
the wrong SourceRange on a non-canonical IncompleteArrayType. It's a
little tricky, so if the issues I'm talking about aren't obvious, I'd
suggest skipping adding the bracket locations to IncompleteArrayType
for now.

Otherwise, this is looking good!

-Eli

OK, here is the patch revised along your suggestions.

Enea.

I got no feedback on my last revision of the patch and it seems that the patch was not applied. Is there something wrong with it? Or should I just wait a bit more?

Cheers,
Enea.

#16

Nothing wrong with it; I'll take a final look at it and apply it
sometime in the next couple days.

-Eli

#17

This patch looks great to me! FWIW, another contributor has been working on adding expression cloning (see the thread on the AST processing lib), so that we can clean up resources appropriately.

Committed here:

   http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20090706/018767.html

  - Doug