Hi Aaron, Thanks for the feedback! Responses (and one question) inline.
On Nov 23, 2013, at 5:27 AM, Aaron Ballman <[email protected]> wrote: > On Fri, Nov 22, 2013 at 8:01 PM, Ted Kremenek <[email protected]> wrote: >> Author: kremenek >> Date: Fri Nov 22 19:01:34 2013 >> New Revision: 195533 >> >> URL: http://llvm.org/viewvc/llvm-project?rev=195533&view=rev >> Log: >> Add back experimental attribute objc_suppress_protocol_methods (slightly >> renamed). >> >> This is still an experimental attribute, but I wanted it in tree >> for review. It may still get yanked. >> >> This attribute can only be applied to a class @interface, not >> a class extension or category. It does not change the type >> system rules for Objective-C, but rather the implementation checking >> for Objective-C classes that explicitly conform to a protocol. >> During protocol conformance checking, clang recursively searches >> up the class hierarchy for the set of methods that compose >> a protocol. This attribute will cause the compiler to not consider >> the methods contributed by a super class, its categories, and those >> from its ancestor classes. Thus this attribute is used to force >> subclasses to redeclare (and hopefully re-implement) methods if >> they decide to explicitly conform to a protocol where some of those >> methods may be provided by a super class. >> >> This attribute intentionally leaves out properties, which are associated >> with state. This attribute only considers methods (at least right now) >> that are non-property accessors. These represent methods that "do something" >> as dictated by the protocol. This may be further refined, and this >> should be considered a WIP until documentation gets written or this >> gets removed. >> >> Added: >> cfe/trunk/test/SemaObjC/protocols-suppress-conformance.m >> Modified: >> cfe/trunk/include/clang/AST/DeclObjC.h >> cfe/trunk/include/clang/Basic/Attr.td >> cfe/trunk/lib/AST/DeclObjC.cpp >> cfe/trunk/lib/Sema/SemaDeclAttr.cpp >> cfe/trunk/lib/Sema/SemaDeclObjC.cpp >> >> Modified: cfe/trunk/include/clang/AST/DeclObjC.h >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/DeclObjC.h?rev=195533&r1=195532&r2=195533&view=diff >> ============================================================================== >> --- cfe/trunk/include/clang/AST/DeclObjC.h (original) >> +++ cfe/trunk/include/clang/AST/DeclObjC.h Fri Nov 22 19:01:34 2013 >> @@ -1143,7 +1143,8 @@ public: >> ObjCMethodDecl *lookupMethod(Selector Sel, bool isInstance, >> bool shallowCategoryLookup = false, >> bool followSuper = true, >> - const ObjCCategoryDecl *C = 0) const; >> + const ObjCCategoryDecl *C = 0, >> + const ObjCProtocolDecl *P = 0) const; >> >> /// Lookup an instance method for a given selector. >> ObjCMethodDecl *lookupInstanceMethod(Selector Sel) const { >> >> Modified: cfe/trunk/include/clang/Basic/Attr.td >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/Attr.td?rev=195533&r1=195532&r2=195533&view=diff >> ============================================================================== >> --- cfe/trunk/include/clang/Basic/Attr.td (original) >> +++ cfe/trunk/include/clang/Basic/Attr.td Fri Nov 22 19:01:34 2013 >> @@ -642,6 +642,12 @@ def ObjCRootClass : InheritableAttr { >> let Subjects = [ObjCInterface]; >> } >> >> +def ObjCSuppressProtocol : InheritableAttr { >> + let Spellings = [GNU<"objc_suppress_protocol_methods">]; >> + let Subjects = [ObjCInterface]; >> + let Args = [IdentifierArgument<"Protocol", 1>]; > > This isn't actually optional in practice. It took me a minute to understand this comment, and then I realized the “1” indicated that the argument was optional. I’m not certain how I missed that. You had commented on this somewhat in your original feedback, and I realized I didn’t quite understand what you meant. Thanks for pointing this out. >> +} >> + >> def Overloadable : Attr { >> let Spellings = [GNU<"overloadable">]; >> let Subjects = [Function]; >> >> Modified: cfe/trunk/lib/AST/DeclObjC.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/DeclObjC.cpp?rev=195533&r1=195532&r2=195533&view=diff >> ============================================================================== >> --- cfe/trunk/lib/AST/DeclObjC.cpp (original) >> +++ cfe/trunk/lib/AST/DeclObjC.cpp Fri Nov 22 19:01:34 2013 >> @@ -460,7 +460,8 @@ ObjCMethodDecl *ObjCInterfaceDecl::looku >> bool isInstance, >> bool shallowCategoryLookup, >> bool followSuper, >> - const ObjCCategoryDecl *C) >> const >> + const ObjCCategoryDecl *C, >> + const ObjCProtocolDecl *P) >> const >> { >> // FIXME: Should make sure no callers ever do this. >> if (!hasDefinition()) >> @@ -473,6 +474,22 @@ ObjCMethodDecl *ObjCInterfaceDecl::looku >> LoadExternalDefinition(); >> >> while (ClassDecl) { >> + // If we are looking for a method that is part of protocol conformance, >> + // check if the superclass has been marked to suppress conformance >> + // of that protocol. >> + if (P && ClassDecl->hasAttrs()) { >> + const AttrVec &V = ClassDecl->getAttrs(); >> + const IdentifierInfo *PI = P->getIdentifier(); >> + for (AttrVec::const_iterator I = V.begin(), E = V.end(); I != E; ++I) >> { >> + if (const ObjCSuppressProtocolAttr *A = >> + dyn_cast<ObjCSuppressProtocolAttr>(*I)){ > > I think this loop could be improved by using a specific_attr_iterator. Thanks! > >> + if (A->getProtocol() == PI) { >> + return 0; >> + } >> + } >> + } >> + } >> + >> if ((MethodDecl = ClassDecl->getMethod(Sel, isInstance))) >> return MethodDecl; >> >> >> Modified: cfe/trunk/lib/Sema/SemaDeclAttr.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclAttr.cpp?rev=195533&r1=195532&r2=195533&view=diff >> ============================================================================== >> --- cfe/trunk/lib/Sema/SemaDeclAttr.cpp (original) >> +++ cfe/trunk/lib/Sema/SemaDeclAttr.cpp Fri Nov 22 19:01:34 2013 >> @@ -2134,6 +2134,28 @@ static void handleObjCRootClassAttr(Sema >> Attr.getAttributeSpellingListIndex())); >> } >> >> +static void handleObjCSuppresProtocolAttr(Sema &S, Decl *D, >> + const AttributeList &Attr) { >> + if (!isa<ObjCInterfaceDecl>(D)) { >> + S.Diag(Attr.getLoc(), diag::err_attribute_wrong_decl_type) >> + << Attr.getName() << ExpectedObjectiveCInterface; >> + return; >> + } >> + >> + IdentifierLoc *Parm = (Attr.getNumArgs() == 1 && Attr.isArgIdent(0)) >> + ? Attr.getArgAsIdent(0) : 0; > > No need to check for getNumargs once you rectify Attr.td Right. > >> + >> + if (!Parm) { >> + S.Diag(D->getLocStart(), diag::err_objc_attr_not_id) << Attr.getName() >> << 1; > > You're not actually checking whether this is an id or not. > Isn’t that what Attr.isArgIdent() does? >> + return; >> + } >> + >> + D->addAttr(::new (S.Context) >> + ObjCSuppressProtocolAttr(Attr.getRange(), S.Context, >> Parm->Ident, >> + >> Attr.getAttributeSpellingListIndex())); >> +} >> + >> + >> static void handleObjCRequiresPropertyDefsAttr(Sema &S, Decl *D, >> const AttributeList &Attr) { >> if (!isa<ObjCInterfaceDecl>(D)) { >> @@ -4713,7 +4735,10 @@ static void ProcessDeclAttribute(Sema &S >> case AttributeList::AT_ObjCRootClass: >> handleObjCRootClassAttr(S, D, Attr); >> break; >> - case AttributeList::AT_ObjCRequiresPropertyDefs: >> + case AttributeList::AT_ObjCSuppressProtocol: >> + handleObjCSuppresProtocolAttr(S, D, Attr); >> + break; >> + case AttributeList::AT_ObjCRequiresPropertyDefs: >> handleObjCRequiresPropertyDefsAttr (S, D, Attr); >> break; >> case AttributeList::AT_Unused: handleUnusedAttr (S, D, Attr); >> break; >> >> Modified: cfe/trunk/lib/Sema/SemaDeclObjC.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclObjC.cpp?rev=195533&r1=195532&r2=195533&view=diff >> ============================================================================== >> --- cfe/trunk/lib/Sema/SemaDeclObjC.cpp (original) >> +++ cfe/trunk/lib/Sema/SemaDeclObjC.cpp Fri Nov 22 19:01:34 2013 >> @@ -1667,7 +1667,9 @@ void Sema::CheckProtocolMethodDefs(Sourc >> (!Super || !Super->lookupMethod(method->getSelector(), >> true /* instance */, >> false /* shallowCategory */, >> - true /* followsSuper */))) { >> + true /* followsSuper */, >> + NULL /* category */, >> + PDecl /* protocol */))) { >> // If a method is not implemented in the category implementation >> but >> // has been declared in its primary class, superclass, >> // or in one of their protocols, no need to issue the warning. >> @@ -1703,7 +1705,9 @@ void Sema::CheckProtocolMethodDefs(Sourc >> (!Super || !Super->lookupMethod(method->getSelector(), >> false /* class method */, >> false /* shallowCategoryLookup */, >> - true /* followSuper */))) { >> + true /* followSuper */, >> + NULL /* category */, >> + PDecl /* protocol */))) { >> // See above comment for instance method lookups. >> if (C && IDecl->lookupMethod(method->getSelector(), >> false /* class */, >> >> Added: cfe/trunk/test/SemaObjC/protocols-suppress-conformance.m >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaObjC/protocols-suppress-conformance.m?rev=195533&view=auto >> ============================================================================== >> --- cfe/trunk/test/SemaObjC/protocols-suppress-conformance.m (added) >> +++ cfe/trunk/test/SemaObjC/protocols-suppress-conformance.m Fri Nov 22 >> 19:01:34 2013 >> @@ -0,0 +1,79 @@ >> +// RUN: %clang_cc1 -triple x86_64-apple-darwin11 -fsyntax-only -verify %s >> -Wno-objc-root-class >> + >> +@protocol Protocol >> +- (void) theBestOfTimes; // expected-note {{method 'theBestOfTimes' >> declared here}} >> +@property (readonly) id theWorstOfTimes; // expected-note {{property >> declared here}} >> +@end >> + >> +// In this example, the root class provides all the methods for >> +// a protocol, and the immediate subclass adopts the attribute. >> +// >> +// The further subclasses should not have access to the root class's >> +// methods for checking protocol conformance. >> +// >> +// ClassC states protocol conformance, but does not redeclare the method. >> +// For this case we get a warning. >> +// >> +// ClassD states protocol conformance, but does redeclare the method. >> +// For this case we do not get a warning. >> +// >> + >> +@interface ClassA <Protocol> >> +- (void) theBestOfTimes; >> +//@property (readonly) id theWorstOfTimes; >> +@end >> + >> +__attribute__((objc_suppress_protocol_methods(Protocol))) @interface ClassB >> : ClassA @end >> + >> +@interface ClassC : ClassB <Protocol> @end // expected-note {{required for >> direct or indirect protocol 'Protocol'}} >> + >> +@interface ClassD : ClassB <Protocol> >> +- (void) theBestOfTimes; >> +@property (readonly) id theWorstOfTimes; >> +@end >> + >> +@implementation ClassA // expected-warning {{auto property synthesis will >> not synthesize property declared in a protocol}} >> +- (void) theBestOfTimes {} >> +@end >> + >> +@implementation ClassC @end // expected-warning {{method 'theBestOfTimes' >> in protocol not implemented}} >> + >> +@implementation ClassD // no-warning >> +- (void) theBestOfTimes {} >> +@end >> + >> +// In this example, the class both conforms to the protocl and adopts >> +// the attribute. This illustrates that the attribute does not >> +// interfere with the protocol conformance checking for the class >> +// itself. >> +__attribute__((objc_suppress_protocol_methods(Protocol))) >> +@interface AdoptsAndConforms <Protocol> >> +- (void) theBestOfTimes; >> +@property (readonly) id theWorstOfTimes; >> +@end >> + >> +@implementation AdoptsAndConforms // no-warning >> +- (void) theBestOfTimes {} >> +@end >> + >> +// This attribute cannot be added to a class extension or category. >> +@interface ClassE >> +-(void) theBestOfTimes; >> +@end >> + >> +__attribute__((objc_supress_protocol(Protocol))) >> +@interface ClassE () @end // expected-error {{attributes may not be >> specified on a category}} >> + >> +__attribute__((objc_supress_protocol(Protocol))) >> +@interface ClassE (MyCat) @end // expected-error {{attributes may not be >> specified on a category}} >> + >> +// The attribute requires one or more identifiers. >> +__attribute__((objc_suppress_protocol_methods())) >> +@interface ClassF @end // expected-error {{parameter of >> 'objc_suppress_protocol_methods' attribute must be a single name of an >> Objective-C protocol}} > > This test will be improved once you remove the optional bit from Attr.td Indeed, thanks. > >> + >> +// The attribute requires one or more identifiers. >> +__attribute__((objc_suppress_protocol_methods(ProtoA, ProtoB))) // >> expected-error {{use of undeclared identifier 'ProtoB'}} > > This test should be complaining about the second argument not that it > doesn't know what second arg is. Is there a way to get this checking automatic via changes in Attr.td? > >> +@interface ClassG @end >> +__attribute__((objc_suppress_protocol_methods(1+2))) >> +@interface ClassH @end // expected-error {{parameter of >> 'objc_suppress_protocol_methods' attribute must be a single name of an >> Objective-C protocol}} >> + >> \ No newline at end of file > > Missing a newline here > > ~Aaron
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
