[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

[Chris Lattner]

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.

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

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

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.

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.

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

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