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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
