[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 3:04 PM, Douglas Gregor wrote:

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

Every method type requires a parameter name. So, this is illegal to start with.

% $CLANG -c t.m
t.m:2:26: error: expected identifier
- method:(id) second:(id);
                                            ^
1 error generated.
- fariborz

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