<html><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On Dec 12, 2007, at 5:03 PM, Chris Lattner wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="word-wrap: break-word; -khtml-nbsp-mode: space; -khtml-line-break: after-white-space; "><div><blockquote type="cite"><div><blockquote type="cite">3. There are inconsistencies in the ObjC class interfaces. For example, it is ObjcInterfaceDecl::getNumInstanceVariables but it is ObjcImplementationDecl::getImplDeclNumIvars. This is strangeness. Please pick a set of shorthand (e.g. Inst for instance, Proto for protocol, Meth for method, Vars for variables etc) and use it consistently. Many method names are really long. getNumInstanceVariables should get getNumInstVars() or getNumIVars().<br><br></blockquote><div><br class="webkit-block-placeholder"></div>I agree with removing the inconsistency. There are two consistency issues here: (1) the ObjC AST API's should be self consistent. (2) the ObjC AST API's should be consistent with the C AST API's.</div></blockquote><div><br class="khtml-block-placeholder"></div>Ok.</div><div><br><blockquote type="cite"><div>That said, I think shortening the names (and coming up with a bunch of abbreviations) is questionable. For example, the type checker uses "getCanonicalType" all over the place. Here is an excerpt...</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: #3f6e74">QualType</span> <span style="color: #3f6e74">lhs</span> = lhsExpr->getType().getCanonicalType().getUnqualifiedType();</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">QualType</span> rhs = rhsExpr->getType().getCanonicalType().getUnqualifiedType();</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>From my perspective, the Type accessors have long names. We could consider converting them to...</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: rgb(63, 110, 116); ">QualType</span> <span style="color: rgb(63, 110, 116); ">lhs</span> = lhsExpr->getType().getCanonType().getUnqualType();</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); ">QualType</span> rhs = rhsExpr->getType().getCanonType().getUnqualType();</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> </div><div><div>From my perspective, this would be the wrong decision (since the original names are "good").</div></div></blockquote><div><br class="khtml-block-placeholder"></div>Ok.</div><div><br><blockquote type="cite"><div><div>I general, I don't believe shortening names just for the sake of shortening is the right decision. The names can certainly be improved. I just don't see a general/good solution to your complaint that "Many method names are really long". </div></div></blockquote><div><br class="khtml-block-placeholder"></div><div>I'm much more concerned with consistency than terseness, I'm happy to go with consistent but long names :)</div><br></div></div></blockquote><div><br class="webkit-block-placeholder"></div>Great. For the AST's, I'd like to stay away from terse, overly abbreviated names. For API's that are less public, I don't care as much...</div><div><br><blockquote type="cite"><div style="word-wrap: break-word; -khtml-nbsp-mode: space; -khtml-line-break: after-white-space; "><div><blockquote type="cite"><div><blockquote type="cite">4. There are 4 or 5 implementations of lookupInstanceMethod, and they all do different things. :( Please pick a name that is more specific to the various classes that explains how it searches. For example, ObjcImplementationDecl::lookupInstanceMethod could be ObjcImplementationDecl::findDefinedInstMethod(), which makes it more clear that it only returns things *defined in the @implementation*.<br><br></blockquote><div><br class="webkit-block-placeholder"></div>I disagree with your assertion that "they all do different things. :(". From my perspective, using polymorphism here makes sense.</div></blockquote><div><br class="khtml-block-placeholder"></div><div>Some do a "shallow" lookup, others walk the inheritance chain. It should be clear from their name what they do.</div><br><blockquote type="cite"><div><blockquote type="cite">5. Related, I think you should split ObjcProtocolDecl::lookupInstanceMethod up into two things: one that searches locally, and one that walks the "referenced protocols", calling the local search on each one. Likewise for any similar ones.<br><br></blockquote><div><br class="webkit-block-placeholder"></div>I think having different ways to search is fine. Off hand, I don't know if I prefer a "knob" (on an existing API) or a new API. To some extent is depends on how we resolve the naming/polymorphism issues above.</div></blockquote><div><br class="khtml-block-placeholder"></div>Please split it into two functions, where one calls the other. instead of "lookupInstanceMethod" how about getinstanceMethodBySelector() or something? Wow that name is long ;-)</div><div><br></div></div></blockquote><div><br class="webkit-block-placeholder"></div>I understand what you looking for...I'll consider this approach and make a proposal. </div><div><br class="webkit-block-placeholder"></div><div>snaroff</div><div><br><blockquote type="cite"><div style="word-wrap: break-word; -khtml-nbsp-mode: space; -khtml-line-break: after-white-space; "><div><blockquote type="cite"><div><blockquote type="cite">6. One specific question is that ObjcInterfaceDecl::lookupInstanceMethod scans methods immediately referenced in protocols, but it doesn't scan protocol's parent protocols. Is this correct? Either way, please change it to call the methods added in step #5.<br><br></blockquote>I agree with you...I don't believe this is correct. ObjcInterfaceDecl should likely query the protocol directly (rather than iterate over the protocols instance methods directly).</div></blockquote><div><br class="khtml-block-placeholder"></div>Ok.</div><br><div>Thanks Steve,</div><div><br class="khtml-block-placeholder"></div><div>-Chris</div></div></blockquote></div><br></body></html>