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

[Ted Kremenek]

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.

> 
>> 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.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131127/5076d003/attachment.html>