[cfe-commits] r164047 - in /cfe/trunk: include/clang/Basic/DiagnosticParseKinds.td lib/Parse/ParseObjc.cpp test/SemaObjC/unused.m test/SemaObjC/warning-missing-selector-name.m

[Douglas Gregor]

On Sep 17, 2012, at 3:01 PM, jahanian <fjahanian at apple.com> wrote:

> 
> On Sep 17, 2012, at 2:51 PM, Douglas Gregor wrote:
> 
>> 
>> On Sep 17, 2012, at 12:15 PM, Fariborz Jahanian <fjahanian at apple.com> wrote:
>> 
>>> Author: fjahanian
>>> Date: Mon Sep 17 14:15:26 2012
>>> New Revision: 164047
>>> 
>>> URL: http://llvm.org/viewvc/llvm-project?rev=164047&view=rev
>>> Log:
>>> objective-C: issue warning when there is no whitespace
>>> between objc method parameter name and colon.
>>> // rdar://12263549
>>> 
>>> Added:
>>>  cfe/trunk/test/SemaObjC/warning-missing-selector-name.m
>>> Modified:
>>>  cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td
>>>  cfe/trunk/lib/Parse/ParseObjc.cpp
>>>  cfe/trunk/test/SemaObjC/unused.m
>>> 
>>> Modified: cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td
>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td?rev=164047&r1=164046&r2=164047&view=diff
>>> ==============================================================================
>>> --- cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td (original)
>>> +++ cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td Mon Sep 17 14:15:26 2012
>>> @@ -213,6 +213,12 @@
>>> "expected ';' after static_assert">;
>>> def err_expected_semi_for : Error<"expected ';' in 'for' statement specifier">;
>>> def err_expected_colon_after : Error<"expected ':' after %0">;
>>> +def missing_selector_name : Warning<
>>> +  "parameter name used as selector"
>>> +  " may result in incomplete method selector name">,
>>> +  InGroup<DiagGroup<"missing-argument-name-in-selector">>;
>> 
>> I find this a little hard to parse. How about
>> 
>> 	"%0 used as the name of the previous parameter rather than as part of the selector"
>> 
>>> +def note_missing_argument_name : Note<
>>> +  "did you mean to use %0 as the selector name instead of %1">;
>> 
>> 	"introduce a parameter name to make %0 part of the selector"
>> 
>> and perhaps a note that specifies how to silence this warning, e.g.
>> 
>> 	"remove whitespace after ')' to use %0 the parameter name and have an empty entry in the selector"
>> 
>>> def err_label_end_of_compound_statement : Error<
>>> "label at end of compound statement: expected statement">;
>>> def err_address_of_label_outside_fn : Error<
>>> 
>>> Modified: cfe/trunk/lib/Parse/ParseObjc.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/ParseObjc.cpp?rev=164047&r1=164046&r2=164047&view=diff
>>> ==============================================================================
>>> --- cfe/trunk/lib/Parse/ParseObjc.cpp (original)
>>> +++ cfe/trunk/lib/Parse/ParseObjc.cpp Mon Sep 17 14:15:26 2012
>>> @@ -18,6 +18,7 @@
>>> #include "clang/Sema/PrettyDeclStackTrace.h"
>>> #include "clang/Sema/Scope.h"
>>> #include "llvm/ADT/SmallVector.h"
>>> +#include "llvm/ADT/StringExtras.h"
>>> using namespace clang;
>>> 
>>> 
>>> @@ -1031,7 +1032,7 @@
>>>                           Scope::FunctionPrototypeScope|Scope::DeclScope);
>>> 
>>> AttributePool allParamAttrs(AttrFactory);
>>> -  
>>> +  bool warnSelectorName = false;
>>> while (1) {
>>>   ParsedAttributes paramAttrs(AttrFactory);
>>>   Sema::ObjCArgInfo ArgInfo;
>>> @@ -1102,6 +1103,13 @@
>>>   SelIdent = ParseObjCSelectorPiece(selLoc);
>>>   if (!SelIdent && Tok.isNot(tok::colon))
>>>     break;
>>> +    if (MethodDefinition && !SelIdent) {
>> 
>> Why does this only apply to method definitions?
> 
> It only concerns the class implementor, so it goes in method definition.
> Putting it in method declaration creates warnings that users of the method have no control over.

So we wouldn't diagnose the frightening problem in this code?

@interface A
- method:(id) second:(id);
@end

@interface A
- method:(id)first second:(id)second;
@end

>> 
>>> +      SourceLocation ColonLoc = Tok.getLocation();
>>> +      if (PP.getLocForEndOfToken(ArgInfo.NameLoc) == ColonLoc) {
>>> +        warnSelectorName = true;
>>> +        Diag(ArgInfo.NameLoc, diag::missing_selector_name);
>>> +      }
>>> +    }
>> 
>> Also check whether that there is space between the ')' from the previous parameter and the name? That way, we will diagnose
>> 
>> 	- method:(id)foo :blah
> We don't want to diagnose here. User used a space intentionally for no selector name (this is the heuristic).

Sorry, I mistyped. I agree that we do not want to diagnose here.

>> 
>> but we will diagnose
>> 
>> 	- method:(id) foo:blah
> 
> Yes, we diagnose this.

Right, this part is fine. Sorry for the noise!

	- Doug