[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
clattner at apple.com
Wed Dec 12 17:03:44 PST 2007
>> 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>
More information about the cfe-commits
mailing list