<html><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On Oct 8, 2008, at 4:20 PM, Argiris Kirtzidis wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><span class="Apple-style-span" style="border-collapse: separate; color: rgb(0, 0, 0); font-family: Helvetica; font-size: 14px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-border-horizontal-spacing: 0px; -webkit-border-vertical-spacing: 0px; -webkit-text-decorations-in-effect: none; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0; ">After the subtle bug that was uncovered here:<br><a href="http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20081006/008095.html">http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20081006/008095.html</a><br>I think that VariableArrayTypes should be unique'd based on the<span class="Apple-converted-space"> </span><br>expression that represents the size.<br><br>The comments of over VariableArrayType class mention:<br><br><blockquote type="cite">/// Note: VariableArrayType's aren't uniqued (since the expressions<span class="Apple-converted-space"> </span><br></blockquote><blockquote type="cite">aren't) and<br></blockquote><blockquote type="cite">/// should not be: two lexically equivalent variable array types could<span class="Apple-converted-space"> </span><br></blockquote><blockquote type="cite">mean<br></blockquote><blockquote type="cite">/// different things, for example, these variables do not have the<span class="Apple-converted-space"> </span><br></blockquote><blockquote type="cite">same type<br></blockquote><blockquote type="cite">/// dynamically:<br></blockquote><blockquote type="cite">///<br></blockquote><blockquote type="cite">/// void foo(int x) {<br></blockquote><blockquote type="cite">/// int Y[x];<br></blockquote><blockquote type="cite">/// ++x;<br></blockquote><blockquote type="cite">/// int Z[x];<br></blockquote><blockquote type="cite">/// }<br></blockquote><br>But those two '[x]' expressions are different DeclRefExpr objects.<br>Not unique'ing the VariableArrayTypes doesn't seem to have any benefit,<span class="Apple-converted-space"> </span><br>since it's a bug to create two VariableArrayTypes with the same Expr*<span class="Apple-converted-space"> </span><br>(they will both try to destroy it).<br>And in general it's unintuitive to have Sema::GetTypeForDeclarator<span class="Apple-converted-space"> </span><br>create different Type* objects each time it gets called with the exact<span class="Apple-converted-space"> </span><br>same declarator.<br></span></blockquote></div><br><div>Hi Argiris,</div><div><br></div><div>Your point makes a lot of sense. By definition the types will be unique since they refer to distinct expressions.</div><div><br></div><div>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).</div><div><br></div><div>Thoughts?</div><div><br></div><div>Ted</div></body></html>