r195533 - Add back experimental attribute objc_suppress_protocol_methods (slightly renamed).
Aaron Ballman
aaron at aaronballman.com
Sun Nov 24 08:00:42 PST 2013
On Sat, Nov 23, 2013 at 5:56 PM, Ted Kremenek <kremenek at apple.com> wrote:
> 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?
I thought id was something more special than just "any identifier?" Eg)
int i;
__attribute__((objc_suppress_protocol_methods(i)))
@interface foo
@end
There would be no error fired here, but based on the error text, it
seems like there should be one.
>
> + 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?
Once the optional bit is removed from Attr.td, this will happen automatically.
>
>
> + 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
>
>
~Aaron
More information about the cfe-commits
mailing list