r195533 - Add back experimental attribute objc_suppress_protocol_methods (slightly renamed).
Ted Kremenek
kremenek at apple.com
Sat Nov 23 14:56:02 PST 2013
Hi Aaron,
Thanks for the feedback! Responses (and one question) inline.
On Nov 23, 2013, at 5:27 AM, Aaron Ballman <aaron at aaronballman.com> wrote:
> On Fri, Nov 22, 2013 at 8:01 PM, Ted Kremenek <kremenek at apple.com> 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
>> +
>> + at protocol Protocol
>> +- (void) theBestOfTimes; // expected-note {{method 'theBestOfTimes' declared here}}
>> + at property (readonly) id theWorstOfTimes; // expected-note {{property declared here}}
>> + at 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.
>> +//
>> +
>> + at interface ClassA <Protocol>
>> +- (void) theBestOfTimes;
>> +//@property (readonly) id theWorstOfTimes;
>> + at end
>> +
>> +__attribute__((objc_suppress_protocol_methods(Protocol))) @interface ClassB : ClassA @end
>> +
>> + at interface ClassC : ClassB <Protocol> @end // expected-note {{required for direct or indirect protocol 'Protocol'}}
>> +
>> + at interface ClassD : ClassB <Protocol>
>> +- (void) theBestOfTimes;
>> + at property (readonly) id theWorstOfTimes;
>> + at end
>> +
>> + at implementation ClassA // expected-warning {{auto property synthesis will not synthesize property declared in a protocol}}
>> +- (void) theBestOfTimes {}
>> + at end
>> +
>> + at implementation ClassC @end // expected-warning {{method 'theBestOfTimes' in protocol not implemented}}
>> +
>> + at implementation ClassD // no-warning
>> +- (void) theBestOfTimes {}
>> + at 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)))
>> + at interface AdoptsAndConforms <Protocol>
>> +- (void) theBestOfTimes;
>> + at property (readonly) id theWorstOfTimes;
>> + at end
>> +
>> + at implementation AdoptsAndConforms // no-warning
>> +- (void) theBestOfTimes {}
>> + at end
>> +
>> +// This attribute cannot be added to a class extension or category.
>> + at interface ClassE
>> +-(void) theBestOfTimes;
>> + at end
>> +
>> +__attribute__((objc_supress_protocol(Protocol)))
>> + at interface ClassE () @end // expected-error {{attributes may not be specified on a category}}
>> +
>> +__attribute__((objc_supress_protocol(Protocol)))
>> + at 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()))
>> + at 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?
>
>> + at interface ClassG @end
>> +__attribute__((objc_suppress_protocol_methods(1+2)))
>> + at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131123/3fbc3d45/attachment.html>
More information about the cfe-commits
mailing list