[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

[jahanian]

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.

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

> 
> but we will diagnose
> 
> 	- method:(id) foo:blah

Yes, we diagnose this.
- fariborz