<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 12:15 AM, Chris Lattner wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite">On Nov 10, 2007, at 4:10 PM, Steve Naroff wrote:<br><blockquote type="cite">Author: snaroff<br></blockquote><blockquote type="cite">Date: Sat Nov 10 18:10:47 2007<br></blockquote><blockquote type="cite">New Revision: 43989<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">URL: <a href="http://llvm.org/viewvc/llvm-project?rev=43989&view=rev">http://llvm.org/viewvc/llvm-project?rev=43989&view=rev</a><br></blockquote><blockquote type="cite">Log:<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">Teach Sema::ActOnInstanceMessage() about private methods. That is, methods declared in an implementation (but not listed in the interface).<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">+// lookupInstanceMethod - This method returns an instance method by looking in<br></blockquote><blockquote type="cite">+// the class implementation. Unlike interfaces, we don't look outside the<br></blockquote><blockquote type="cite">+// implementation.<br></blockquote><blockquote type="cite">+ObjcMethodDecl *ObjcImplementationDecl::lookupInstanceMethod(Selector &Sel) {<br></blockquote><blockquote type="cite">+ ObjcMethodDecl **methods = getInstanceMethods();<br></blockquote><blockquote type="cite">+ int methodCount = getNumInstanceMethods();<br></blockquote><blockquote type="cite">+ for (int i = 0; i < methodCount; ++i) {<br></blockquote><blockquote type="cite">+ if (methods[i]->getSelector() == Sel) {<br></blockquote><blockquote type="cite">+ return methods[i];<br></blockquote><blockquote type="cite">+ }<br></blockquote><blockquote type="cite">+ }<br></blockquote><blockquote type="cite">+ return NULL;<br></blockquote><blockquote type="cite">+}<br></blockquote><br>Hi Steve,<br><br>I started making some changes related to this, but I need your help.<br><br>Please make the following changes:<br><br>1. Introduce iterators for all of the "accessors" for the various lists in the ObjC data structures. For example ObjcInterfaceDecl::getInstanceMethods should go away. This is important because the type returned by getInstanceMethods is currently incorrect (even though the method is const, the returned pointer is not correctly const qualified).<br></blockquote><div><br></div></div><div><blockquote type="cite">2. Switch clients over from using stuff like getInstanceMethods to use the iterators and remove the old interfaces. The various objc class should not expose to their clients that how they happen to represent these lists.<br><br></blockquote><div><br class="webkit-block-placeholder"></div><div>Absolutely (to items 1/2).</div><div><br class="webkit-block-placeholder"></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><div><br class="webkit-block-placeholder"></div><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><br class="webkit-block-placeholder"></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><br></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><div><br><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><div><br><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><div><br class="webkit-block-placeholder"></div>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><div><br><blockquote type="cite">7. We need documentation on methods like ObjcInterfaceDecl::addMethods that explicitly say that they don't take ownership of the pointers passed in.<br><br></blockquote><div><br class="webkit-block-placeholder"></div>o.k.</div><div><br><blockquote type="cite">8. This is a very common pattern in the ObjC classes:<br><br>... Class ...<br> /// category instance methods<br> ObjcMethodDecl **InstanceMethods; // Null if not defined<br> int NumInstanceMethods; // -1 if not defined<br>...<br><br> ... header exposes iterator interface...<br><br> NumInstanceMethods = numInsMembers;<br> if (numInsMembers) {<br> InstanceMethods = new ObjcMethodDecl*[numInsMembers];<br> memcpy(InstanceMethods, insMethods, numInsMembers*sizeof(ObjcMethodDecl*));<br> }<br><br><br>I don't like how the clients are expected to check for null/-1 to use this API. I think that it should just expose an iterator interface, which will give an empty range instead of null/-1. The client would be forced to check something else to distinguish between an interface forward def and and interface definition with no inst methods.<br><br>Given that change, I think it may make sense to introduce a new simple class that wraps this concept. This would give us:<br><br> MyDynamicArray<ObjcMethodDecl> InstMethods;<br><br>instead of the two fields above.<br><br><br>This is a lot of stuff, but it can be done one at a time. I think it's really important to get the APIs right before there are more clients. Please lemme know if you disagree with any of the above of course,<br><br>-Chris<br></blockquote></div><br></body></html>