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

	- Doug