[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]

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

Ok.

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

Ok.

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

I'm much more concerned with consistency than terseness, I'm happy to  
go with consistent but long names :)

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

Some do a "shallow" lookup, others walk the inheritance chain.  It  
should be clear from their name what they do.

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

Please split it into two functions, where one calls the other.   
instead of "lookupInstanceMethod" how about  
getinstanceMethodBySelector() or something?  Wow that name is long ;-)

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

Ok.

Thanks Steve,

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