[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 5:03 PM, Chris Lattner wrote:

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

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

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

I understand what you looking for...I'll consider this approach and  
make a proposal.

snaroff

>>> 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/82dbd589/attachment.html>