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

> 
> On Sep 17, 2012, at 3:09 PM, jahanian <fjahanian at apple.com> wrote:
> 
>> 
>> 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
> 
> Corrected example:
> 
> interface A
> - method:(id) second:(id)second;
> @end
> 
> @implementation A
> - method:(id)first second:(id)second { }
> @end
> 
> Aside from the general "incomplete implementation" warning, this won't get diagnosed by your patch because it's the @interface that has the problem, but we're only diagnosing for @implementations.
In r164077.
- fariborz

> 
> 	- Doug