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