[cfe-commits] r43989 - in /cfe/trunk: AST/Decl.cpp Sema/Sema.h Sema/SemaDecl.cpp Sema/SemaExpr.cpp include/clang/AST/DeclObjC.h test/Sema/message.m

[Steve Naroff]

On Dec 12, 2007, at 12:15 AM, Chris Lattner wrote:

> On Nov 10, 2007, at 4:10 PM, Steve Naroff wrote:
>> Author: snaroff
>> Date: Sat Nov 10 18:10:47 2007
>> New Revision: 43989
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=43989&view=rev
>> Log:
>>
>> Teach Sema::ActOnInstanceMessage() about private methods. That is,  
>> methods declared in an implementation (but not listed in the  
>> interface).
>>
>> +// lookupInstanceMethod - This method returns an instance method  
>> by looking in
>> +// the class implementation. Unlike interfaces, we don't look  
>> outside the
>> +// implementation.
>> +ObjcMethodDecl  
>> *ObjcImplementationDecl::lookupInstanceMethod(Selector &Sel) {
>> +  ObjcMethodDecl **methods = getInstanceMethods();
>> +  int methodCount = getNumInstanceMethods();
>> +  for (int i = 0; i < methodCount; ++i) {
>> +    if (methods[i]->getSelector() == Sel) {
>> +      return methods[i];
>> +    }
>> +  }
>> +  return NULL;
>> +}
>
> Hi Steve,
>
> I started making some changes related to this, but I need your help.
>
> Please make the following changes:
>
> 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).

> 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.
>

Absolutely (to items 1/2).

> 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().
>

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.

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...

   QualType lhs = lhsExpr- 
 >getType().getCanonicalType().getUnqualifiedType();
   QualType rhs = rhsExpr- 
 >getType().getCanonicalType().getUnqualifiedType();

 From my perspective, the Type accessors have long names. We could  
consider converting them to...

   QualType lhs = lhsExpr->getType().getCanonType().getUnqualType();
   QualType rhs = rhsExpr->getType().getCanonType().getUnqualType();

 From my perspective, this would be the wrong decision (since the  
original names are "good").

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".

> 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*.
>

I disagree with your assertion that "they all do different things. : 
(". From my perspective, using polymorphism here makes sense.

> 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.
>

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.

> 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.
>

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).

> 7. We need documentation on methods like  
> ObjcInterfaceDecl::addMethods that explicitly say that they don't  
> take ownership of the pointers passed in.
>

o.k.

> 8. This is a very common pattern in the ObjC classes:
>
> ... Class ...
>  /// category instance methods
>  ObjcMethodDecl **InstanceMethods;  // Null if not defined
>  int NumInstanceMethods;  // -1 if not defined
> ...
>
>  ... header exposes iterator interface...
>
>  NumInstanceMethods = numInsMembers;
>  if (numInsMembers) {
>    InstanceMethods = new ObjcMethodDecl*[numInsMembers];
>    memcpy(InstanceMethods, insMethods,  
> numInsMembers*sizeof(ObjcMethodDecl*));
>  }
>
>
> 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.
>
> Given that change, I think it may make sense to introduce a new  
> simple class that wraps this concept.  This would give us:
>
>   MyDynamicArray<ObjcMethodDecl> InstMethods;
>
> instead of the two fields above.
>
>
> 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,
>
> -Chris

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20071212/bf6fbfb5/attachment.html>