<html><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On Nov 16, 2007, at 9:10 AM, Chris Lattner wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><blockquote type="cite">I assume that this is similar to a C++ example like this:<br><br>class foo {<br> int x;<br><br> int bar() { return x; }<br>};<br><br>Why is it more natural to refer to "x" as a field reference than as a declref of the FieldDecl? I know that GCC explicitly does lowering when is parses (making this a field reference off the hidden this/self parameter), but that doesn't seem like the right choice for our ASTs.<br><br>Thoughts?<br></blockquote></div><br><div>Chris,</div><div><br class="webkit-block-placeholder"></div><div>It's a little more complicated than the above example. Consider this pattern from "Sketch"...with the exception of the "copy" variable, all other variable references in this example are represented by "ObjCIvarRefExpr" (_bounds, _isDrawingFill, etc.).</div><div><br class="webkit-block-placeholder"></div><div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; ">- (<span style="color: #aa0d91">id</span>)copyWithZone:(<span style="color: #5c2699">NSZone</span> *)zone {</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; min-height: 14px; "><br></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; color: rgb(0, 116, 0); "><span class="Apple-style-span" style="color: rgb(0, 0, 0); "> <span style="color: #3f6e74">SKTGraphic</span> *copy = [[[<span style="color: #aa0d91">self</span> <span style="color: #2e0d6e">class</span>] <span style="color: #2e0d6e">alloc</span>] <span style="color: #2e0d6e">init</span>];</span></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; "> copy-><span style="color: #5c2699">_bounds</span> = <span style="color: #3f6e74">_bounds</span>;</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; color: rgb(63, 110, 116); "><span style="color: #000000"> copy-></span>_isDrawingFill<span style="color: #000000"> = </span>_isDrawingFill<span style="color: #000000">;</span></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; color: rgb(63, 110, 116); "><span style="color: #000000"> copy-></span>_fillColor<span style="color: #000000"> = [</span>_fillColor<span style="color: #000000"> </span><span style="color: #2e0d6e">copy</span><span style="color: #000000">];</span></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; color: rgb(63, 110, 116); "><span style="color: #000000"> copy-></span>_isDrawingStroke<span style="color: #000000"> = </span>_isDrawingStroke<span style="color: #000000">;</span></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; color: rgb(63, 110, 116); "><span style="color: #000000"> copy-></span>_strokeColor<span style="color: #000000"> = [</span>_strokeColor<span style="color: #000000"> </span><span style="color: #2e0d6e">copy</span><span style="color: #000000">];</span></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; color: rgb(63, 110, 116); "><span style="color: #000000"> copy-></span>_strokeWidth<span style="color: #000000"> = </span>_strokeWidth<span style="color: #000000">;</span></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; "> <span style="color: #aa0d91">return</span> copy;</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; ">}</div></div></div></blockquote><div><br class="webkit-block-placeholder"></div>Ok, thanks for the example!</div><div><br><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div><span class="Apple-style-span" style="-webkit-text-stroke-width: -1; ">We never "lower" the ivar references. The AST reflects the code above. For example, there are no synthesized "MemberExpr's" for the ivar references on the right hand side of the assignment expressions above. ObjCIvarRefExpr represents both patterns. The arrow for the left-hand side ivar references is really part of the ObjC syntax (since the type of "copy" is an interface, not a structure).</span></div></div></div></blockquote><div><br class="webkit-block-placeholder"></div><div>Okay, I'm sorry. When I saw the patch, I thought the addition was to <span class="Apple-style-span" style="font-family: Monaco; font-size: 10px; ">MemberExpr, I didn't realize it was ObjCIvarRefExpr.</span></div><div><br></div><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div><font class="Apple-style-span" face="Monaco" size="2"><span class="Apple-style-span" style="font-size: 10px;"><span class="Apple-style-span" style="font-family: Helvetica; font-size: 12px; "><div>That said, I still think what we have can be improved. From my perspective, ObjCIvarRefExpr could be improved to represent the "common" case more naturally, where we don't need an "Expr *" (since "self" is assumed). This would both save space and remove improve unnecessary code from Sema::ActOnIdentifierExpr(). Here's what I am talking about...</div><div><br class="webkit-block-placeholder"></div><div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; "> <span style="color: #aa0d91">if</span> (CurMethodDecl) {</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; "> <span style="color: #3f6e74">ObjcInterfaceDecl</span> *<span style="color: #3f6e74">IFace</span> = CurMethodDecl->getClassInterface();</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; color: rgb(63, 110, 116); "><span style="color: #000000"> </span>ObjcInterfaceDecl<span style="color: #000000"> *</span>clsDeclared<span style="color: #000000">;</span></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; "> <span style="color: #aa0d91">if</span> (<span style="color: #3f6e74">ObjcIvarDecl</span> *<span style="color: #3f6e74">IV</span> = <span style="color: #3f6e74">IFace</span>->lookupInstanceVariable(&<span style="color: #3f6e74">II</span>, <span style="color: #3f6e74">clsDeclared</span>)) {</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; "> <span style="color: #3f6e74">IdentifierInfo</span> &<span style="color: #3f6e74">II</span> = <span style="color: #3f6e74">Context</span>.Idents.<span style="color: #2e0d6e">get</span>(<span style="color: #c41a16">"self"</span>); // unnecessary.</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; "> <span style="color: #3f6e74">ExprResult</span> SelfExpr = <span style="color: #26474b">ActOnIdentifierExpr</span>(<span style="color: #3f6e74">S</span>, <span style="color: #3f6e74">Loc</span>, <span style="color: #3f6e74">II</span>, <span style="color: #aa0d91">false</span>); // ditto.</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; "> <span style="color: #aa0d91">return</span> <span style="color: #aa0d91">new</span> <span style="color: #3f6e74">ObjCIvarRefExpr</span>(<span style="color: #3f6e74">IV</span>, <span style="color: #3f6e74">IV</span>->getType(), <span style="color: #3f6e74">Loc</span>, </div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; "> <span style="color: #aa0d91">static_cast</span><<span style="color: #3f6e74">Expr</span>*>(SelfExpr.<span style="color: #3f6e74">Val</span>), <span style="color: #aa0d91">true</span>, <span style="color: #aa0d91">true</span>);</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; "> }</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; "> }</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; "><br class="webkit-block-placeholder"></div><div><font class="Apple-style-span" face="Monaco" size="2"><span class="Apple-style-span" style="font-size: 10px;"><span class="Apple-style-span" style="font-family: Helvetica; font-size: 12px; "><div>...would become.... </div><div><br class="webkit-block-placeholder"></div></span></span></font></div> </div><div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; "> <span style="color: rgb(170, 13, 145); ">if</span> (CurMethodDecl) {</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; "> <span style="color: rgb(63, 110, 116); ">ObjcInterfaceDecl</span> *<span style="color: rgb(63, 110, 116); ">IFace</span> = CurMethodDecl->getClassInterface();</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; color: rgb(63, 110, 116); "><span style="color: rgb(0, 0, 0); "> </span>ObjcInterfaceDecl<span style="color: rgb(0, 0, 0); "> *</span>clsDeclared<span style="color: rgb(0, 0, 0); ">;</span></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; "> <span style="color: rgb(170, 13, 145); ">if</span> (<span style="color: rgb(63, 110, 116); ">ObjcIvarDecl</span> *<span style="color: rgb(63, 110, 116); ">IV</span> = <span style="color: rgb(63, 110, 116); ">IFace</span>->lookupInstanceVariable(&<span style="color: rgb(63, 110, 116); ">II</span>, <span style="color: rgb(63, 110, 116); ">clsDeclared</span>))</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; "> <span style="color: rgb(170, 13, 145); ">return</span> <span style="color: rgb(170, 13, 145); ">new</span> <span style="color: rgb(63, 110, 116); ">ObjCIvarRefExpr</span>(<span style="color: rgb(63, 110, 116); ">IV</span>, <span style="color: rgb(63, 110, 116); ">IV</span>->getType(), <span style="color: rgb(63, 110, 116); ">Loc);</span> </div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; "> }</div><div><font class="Apple-style-span" face="Monaco" size="2"><span class="Apple-style-span" style="font-size: 10px;"><br class="webkit-block-placeholder"></span></font></div><div><font class="Apple-style-span" face="Monaco" size="2"><span class="Apple-style-span" style="font-size: 10px;"><span class="Apple-style-span" style="font-family: Helvetica; font-size: 12px; "><div>For now, I suggest we make the change above and have "Base" be null for normal ivar references. As extra credit, we could factor this out into two classes for each of the above cases.</div></span></span></font></div></div></span></span></font></div></div></div></blockquote><br></div><div>I think this is a great step in the right direction. However, what is the advantage of using <span class="Apple-style-span" style="font-family: Monaco; font-size: 10px; ">ObjCIvarRefExpr over DeclRefExpr? With the proposed code, you are capturing exactly the same information as DeclRefExpr would. Just replace:</span></div><div><font class="Apple-style-span" face="Monaco" size="2"><span class="Apple-style-span" style="font-size: 10px;"><br class="webkit-block-placeholder"></span></font></div><div><font class="Apple-style-span" face="Monaco" size="2"><span class="Apple-style-span" style="font-size: 10px;"><span style="color: rgb(170, 13, 145); "> return</span> <span style="color: rgb(170, 13, 145); ">new</span> <span style="color: rgb(63, 110, 116); ">ObjCIvarRefExpr</span>(<span style="color: rgb(63, 110, 116); ">IV</span>, <span style="color: rgb(63, 110, 116); ">IV</span>->getType(), <span style="color: rgb(63, 110, 116); ">Loc);</span> </span></font></div><div><font class="Apple-style-span" face="Monaco" size="2"><span class="Apple-style-span" style="font-size: 10px;">with:</span></font></div><div><font class="Apple-style-span" face="Monaco" size="2"><span class="Apple-style-span" style="font-size: 10px;"><span style="color: rgb(170, 13, 145); "> return</span> <span style="color: rgb(170, 13, 145); ">new</span> DeclRefExpr(<span style="color: rgb(63, 110, 116); ">IV</span>, <span style="color: rgb(63, 110, 116); ">IV</span>->getType(), <span style="color: rgb(63, 110, 116); ">Loc);</span> </span></font></div><div><font class="Apple-style-span" face="Monaco" size="2"><span class="Apple-style-span" style="font-size: 10px;"><br class="webkit-block-placeholder"></span></font></div></div></blockquote><div><br class="webkit-block-placeholder"></div>I agree (and came to the same conclusion over my workout this morning).</div><div><br class="webkit-block-placeholder"></div><div>Decision: We replace ObjCIvarRefExpr with MemberExpr and DeclRefExpr.</div><div><br></div><div><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><font class="Apple-style-span" face="Monaco" size="2"><span class="Apple-style-span" style="font-size: 10px;">Also, I consider the rewriter to be somewhat orthogonal to this discussion. This is really about getting the ASTs right. Getting the ASTs right requires being aware of the clients, but simplifying the ASTs is goodness even if any one particular client gets slightly more complex.</span></font></div><div><font class="Apple-style-span" face="Monaco" size="2"><span class="Apple-style-span" style="font-size: 10px;"><br class="webkit-block-placeholder"></span></font></div></div></blockquote><div><br class="webkit-block-placeholder"></div>I totally agree...the AST's need to be "right", independent of any particular application.</div><div><br class="webkit-block-placeholder"></div><div>That said, while this is simplifying, it does increase the complexity of determining if an expression is actually an ivar reference.</div><div><br class="webkit-block-placeholder"></div><div>No biggy, but worth mentioning (since complexity can be viewed from different perspectives...).</div><div><br class="webkit-block-placeholder"></div><div>snaroff</div><div><br class="webkit-block-placeholder"></div><div><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><font class="Apple-style-span" face="Monaco" size="2"><span class="Apple-style-span" style="font-size: 10px;">-Chris</span></font></div><div><br class="webkit-block-placeholder"></div><div><br class="webkit-block-placeholder"></div></div></blockquote></div><br></body></html>