r195533 - Add back experimental attribute objc_suppress_protocol_methods (slightly renamed).

Aaron Ballman aaron at aaronballman.com
Thu Nov 28 08:27:28 PST 2013

On Thu, Nov 28, 2013 at 1:40 AM, Ted Kremenek <kremenek at apple.com> wrote:
> On Nov 27, 2013, at 5:40 AM, Aaron Ballman <aaron at aaronballman.com> wrote:
> On Wed, Nov 27, 2013 at 2:02 AM, Ted Kremenek <kremenek at apple.com> wrote:
> On Nov 24, 2013, at 8:00 AM, Aaron Ballman <aaron at aaronballman.com> wrote:
> 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.
> This is valid.  Objective-C protocols are in their own namespace, and this
> attribute is meant to work regardless of whether or not the protocol is
> visible within the translation unit.  Thus this case is valid.  The ‘i’ here
> for the attribute parameter has nothing to do with ‘int i’.
> I think we're talking past one another though. The diagnostic you have
> will be issued when there is no identifier present (for instance,
> you're given an expression as the sole argument). The diagnostic
> issued says:
> parameter of 'objc_suppress_protocol_methods' attribute must be a
> single name of an Objective-C protocol
> Except you're not checking whether what you receive is a single name
> of an Objective-C protocol. Any identifier will do, such as passing
> int i; That seems confusing to me -- I would assume that if you
> perform the lookup and *know* what you have it not a protocol, then
> you would issue the error. If you perform the lookup and don't find
> anything, I can understand suppressing the diagnostic in that case.
> Does that make more sense?
> Hi Aaron,
> I understand where you are coming from, but it isn’t quite right.  It
> doesn’t matter if the identifier can resolve to a type or a variable at that
> point, because the protocol is allowed to be declared later (or not at all)
> in the translation unit.  To illustrate, I can expand your example just a
> bit:
> int i;
> __attribute__((objc_suppress_protocol_methods(i)))
> @interface foo
> @end
> @protocol i
> @end
> The protocol “i” and the variable “i” will not conflict.  Moreover, the
> protocol “i” need not be visible when we process the attribute.  Thus
> checking if ‘i’ resolves to an identifier that isn’t a protocol isn’t
> useful.
> Simply put, the fact that (a) there are other things with the same
> identifier is irrelevant and (b) the protocol may or may not be visible
> within the translation unit together implies the checking logic that is
> there.  Thus the only thing we can check for is that it is an identifier; if
> it isn’t, a warning indicates that it should be an identifier that (as a
> hint) should name some protocol, but that protocol doesn’t need to be
> declared at the point this attribute is processed.

Ah, now I understand -- thank you for the explanation. My
recommendation is slightly changed then -- instead of using
err_objc_attr_not_id here, I think it'd be more consistent to extend
the existing err_attribute_argument_type to handle your cases.

> This is what I have in Attr.td:
> def ObjCSuppressProtocol : InheritableAttr {
>  let Spellings = [GNU<"objc_suppress_protocol_methods">];
>  let Subjects = [ObjCInterface];
>  let Args = [IdentifierArgument<"Protocol">];
> }
> and this is what we have in the test case:
>  __attribute__((objc_suppress_protocol_methods(ProtoA, ProtoB))) //
> expected-error {{use of undeclared identifier 'ProtoB'}}
> Am I missing something?  From what I can tell the optional bit has been
> removed, and yet the error remains as it is.  For this test case, it is
> intentional that ProtoB is not declared; that’s perfectly fine.  The problem
> is that the attribute takes one argument.
> Ah, this is an error generated from parsing, not sema. As we're
> parsing the args in ParseGNUAttributeArgs, we're calling
> ParseAssignmentExpression on ProtoB, and it's not being found as a
> primary expression, so it generates the diagnostic. A better test in
> this case would be to supply an identifier that can be successfully
> looked up. Sorry for the confusion!
> It sounds like there should be two test cases, not just redefining the
> current one.  Given how this attribute behaves, the second identifier could
> be referencing a protocol that could exist; it just hasn’t been either seen
> yet.  That’s what this test case catches, and it results in a suboptimal
> diagnostic (was you have pointed out) for this completely valid case.

Suddenly, things make more sense. :-) As it stands right now, you can
have a resolved identifier, or an unresolved identifier appear as an
attribute argument. You're asking for an unresolved identifier when
you use IdentifierArgument. This is a good thing for the way your
first argument works. However, we only support unresolved identifiers
as the first argument currently -- from the second argument on, we
resolve all identifiers. You can see this in ParseDecl.cpp in
ParseGNUAttributeArgs -- if the first arg is an identifier and we know
the attribute wants to leave it unresolved, we add it directly to the
argument list, otherwise we call ParseAssignmentExpression and add the
resolved result to the argument list. That's why the error is the way
it is for your test case. I agree that it would be better in this
particular instance if we erred on the number of arguments rather than
whether ProtoB can be resolved or not, and that we should likely have
two test cases here. I would do:

// FIXME: it would be better to diagnose the number of arguments here
instead of the resolved identifier
__attribute__((objc_suppress_protocol_methods(ProtoA, ProtoB))) //
expected-error {{use of undeclared identifier 'ProtoB'}}

__attribute__((objc_suppress_protocol_methods(ProtoA, 1))) //
expected-error {{'objc_suppress_protocol_methods' attribute takes one

At some point, I imagine the parsing will get improved where we can
then remove the FIXME from the test case.



More information about the cfe-commits mailing list