[cfe-commits] r154163 - in /cfe/trunk: include/clang/Basic/DiagnosticParseKinds.td lib/Parse/ParseTemplate.cpp test/FixIt/fixit-cxx0x.cpp test/FixIt/fixit.cpp test/FixIt/no-fixit.cpp test/Parser/cxx-template-decl.cpp

[David Blaikie]

On Sat, Apr 7, 2012 at 12:52 AM, Francois Pichet <pichet2000 at gmail.com> wrote:
> On Fri, Apr 6, 2012 at 1:26 AM, David Blaikie <dblaikie at gmail.com> wrote:
>> Author: dblaikie
>> Date: Fri Apr  6 00:26:43 2012
>> New Revision: 154163
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=154163&view=rev
>> Log:
>> Restrict fixit for missing 'class' in template template parameters.
>>
>> Based on Doug's feedback to r153887 this omits the FixIt if the following token
>> isn't syntactically valid for the context. (not a comma, '...', identifier,
>> '>', or '>>')
>>
>> There's a bunch of work to handle the '>>' case, but it makes for a much more
>> pleasant diagnostic in this case.
>>
>> Added:
>>    cfe/trunk/test/FixIt/no-fixit.cpp
>> Modified:
>>    cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td
>>    cfe/trunk/lib/Parse/ParseTemplate.cpp
>>    cfe/trunk/test/FixIt/fixit-cxx0x.cpp
>>    cfe/trunk/test/FixIt/fixit.cpp
>>    cfe/trunk/test/Parser/cxx-template-decl.cpp
>>
>> Modified: cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td?rev=154163&r1=154162&r2=154163&view=diff
>> ==============================================================================
>> --- cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td (original)
>> +++ cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td Fri Apr  6 00:26:43 2012
>> @@ -479,8 +479,8 @@
>>   "unknown template name %0">;
>>  def err_expected_comma_greater : Error<
>>   "expected ',' or '>' in template-parameter-list">;
>> -def err_expected_class_on_template_template_param : Error<
>> -  "template template parameters require 'class' after the argument list">;
>> +def err_class_on_template_template_param : Error<
>> +  "template template parameter requires 'class' after the parameter list">;
>>  def err_template_spec_syntax_non_template : Error<
>>   "identifier followed by '<' indicates a class template specialization but "
>>   "%0 %select{does not refer to a template|refers to a function "
>>
>> Modified: cfe/trunk/lib/Parse/ParseTemplate.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/ParseTemplate.cpp?rev=154163&r1=154162&r2=154163&view=diff
>> ==============================================================================
>> --- cfe/trunk/lib/Parse/ParseTemplate.cpp (original)
>> +++ cfe/trunk/lib/Parse/ParseTemplate.cpp Fri Apr  6 00:26:43 2012
>> @@ -312,11 +312,15 @@
>>   if (Tok.is(tok::greater))
>>     RAngleLoc = ConsumeToken();
>>   else if (ParseTemplateParameterList(Depth, TemplateParams)) {
>> -    if (!Tok.is(tok::greater)) {
>> +    if (Tok.is(tok::greatergreater)) {
>> +      Tok.setKind(tok::greater);
>> +      Tok.setLocation(Tok.getLocation().getLocWithOffset(1));
>> +    } else if (Tok.is(tok::greater))
>> +      RAngleLoc = ConsumeToken();
>> +    else {
>>       Diag(Tok.getLocation(), diag::err_expected_greater);
>>       return true;
>>     }
>> -    RAngleLoc = ConsumeToken();
>>   }
>>   return false;
>>  }
>> @@ -339,13 +343,13 @@
>>     } else {
>>       // If we failed to parse a template parameter, skip until we find
>>       // a comma or closing brace.
>> -      SkipUntil(tok::comma, tok::greater, true, true);
>> +      SkipUntil(tok::comma, tok::greater, tok::greatergreater, true, true);
>
> Not sure what you are trying to do here but this give the highly
> suspicious MSVC warning:
>
> 107>ParseTemplate.cpp(346): warning C4305: 'argument' : truncation
> from 'clang::tok::TokenKind' to 'bool'
>
> Basically the third argument 'tok::greatergreater' is for a bool
> param; will always be true. Is this the intend?

Not at all intended. Seems I need more test cases for those failure
paths because they all suffer from the same problem (that SkipUntil
has convenience versions for one and two arguments - but not for more
(beyond two you need to use the array version)). Perhaps I'l add a
convenience version for 3 (& might fix up the array version to use
ArrayRef).

I'll also check whether this would've been caught by my improvements
to the implicit conversion warnings we have (I sent it for CR a few
days ago - by coincidence).

Thanks for the pointer/catch,
- David