[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 10:30 AM, David Blaikie <dblaikie at gmail.com> wrote:
> 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).

Fixed (along with some other things I found with my diagnostic
improvements and by inspection) with test cases (& ArrayRef
improvement) in r154325.

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

Yep - the -Wconstant-conversion improvements caught this too. Yay.

Thanks again,
- David