[cfe-commits] Unique'ing VariableArrayType

[Ted Kremenek]

On Oct 8, 2008, at 4:20 PM, Argiris Kirtzidis wrote:

> After the subtle bug that was uncovered here:
> http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20081006/008095.html
> I think that VariableArrayTypes should be unique'd based on the
> expression that represents the size.
>
> The comments of over VariableArrayType class mention:
>
>> /// Note: VariableArrayType's aren't uniqued (since the expressions
>> aren't) and
>> /// should not be: two lexically equivalent variable array types  
>> could
>> mean
>> /// different things, for example, these variables do not have the
>> same type
>> /// dynamically:
>> ///
>> /// void foo(int x) {
>> ///   int Y[x];
>> ///   ++x;
>> ///   int Z[x];
>> /// }
>
> But those two '[x]' expressions are different DeclRefExpr objects.
> Not unique'ing the VariableArrayTypes doesn't seem to have any  
> benefit,
> since it's a bug to create two VariableArrayTypes with the same Expr*
> (they will both try to destroy it).
> And in general it's unintuitive to have Sema::GetTypeForDeclarator
> create different Type* objects each time it gets called with the exact
> same declarator.

Hi Argiris,

Your point makes a lot of sense.  By definition the types will be  
unique since they refer to distinct expressions.

I think there are a few issues worth thinking about.  The first one  
has to do with tying the functionality of the VariableArrayType object  
to the lifetime of its referenced Expr.  If the rewriter (or some  
other refactoring) decides to change the Expr referenced by the  
VariableArrayType, what happens to the VariableArrayType?  If the  
VariableArrayType is uniqued by using the Expr* value, then if that  
Expr is replaced then all of a sudden the VariableArrayType might be  
incorrectly placed in the FoldingSet that we use to unique types  
(within ASTContext).  A refactoring client would have to get this  
corner case right when making changes to an AST, potentially by  
removing the VariableArrayType from the foldingset before changing  
it's underlying Expr and then reinserting it into the FoldingSet after  
changing the underlying Expr.  Requiring this would be strangely  
burdensome on clients changing the AST, nor do we currently expose the  
FoldingSet implementation for uniquing types in the public interface  
of ASTContext (and my belief is that we shouldn't expose this  
implementation detail).

Thoughts?

Ted
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20081008/a077ab34/attachment.html>