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

[Douglas Gregor]

On Apr 5, 2012, at 10:00 AM, David Blaikie <dblaikie at gmail.com> wrote:

> On Tue, Apr 3, 2012 at 8:52 AM, Douglas Gregor <dgregor at apple.com> wrote:
>> 
>> On Apr 2, 2012, at 12:15 PM, David Blaikie wrote:
>> 
>>> Author: dblaikie
>>> Date: Mon Apr  2 14:15:28 2012
>>> New Revision: 153887
>>> 
>>> URL: http://llvm.org/viewvc/llvm-project?rev=153887&view=rev
>>> Log:
>>> Correct error recovery when missing 'class' in a template template parameter.
>>> 
>>> The diagnostic message correctly informs the user that they have omitted the
>>> 'class' keyword, but neither suggests this insertion as a fixit, nor attempts
>>> to recover as if they had provided the keyword.
>>> 
>>> This fixes the recovery, adds the fixit, and adds a separate diagnostic and
>>> corresponding replacement fixit for cases where the user wrote 'struct' or
>>> 'typename' instead of 'class' (suggested by Richard Smith as a possible common
>>> mistake).
>>> 
>>> I'm not sure the diagnostic message for either the original or new cases feel
>>> very Clang-esque, so I'm open to suggestions there. The fixit hints make it
>>> fairly easy to see what's required, though.
>>> 
>>> Modified:
>>>    cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td
>>>    cfe/trunk/lib/Parse/ParseTemplate.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=153887&r1=153886&r2=153887&view=diff
>>> ==============================================================================
>>> --- cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td (original)
>>> +++ cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td Mon Apr  2 14:15:28 2012
>>> @@ -480,6 +480,7 @@
>>> def err_expected_comma_greater : Error<
>>>   "expected ',' or '>' in template-parameter-list">;
>>> def err_expected_class_before : Error<"expected 'class' before '%0'">;
>>> +def err_expected_class_instead : Error<"expected 'class' instead of '%0'">;
>>> 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=153887&r1=153886&r2=153887&view=diff
>>> ==============================================================================
>>> --- cfe/trunk/lib/Parse/ParseTemplate.cpp (original)
>>> +++ cfe/trunk/lib/Parse/ParseTemplate.cpp Mon Apr  2 14:15:28 2012
>>> @@ -540,12 +540,17 @@
>>> 
>>>   // Generate a meaningful error if the user forgot to put class before the
>>>   // identifier, comma, or greater.
>>> -  if (!Tok.is(tok::kw_class)) {
>>> -    Diag(Tok.getLocation(), diag::err_expected_class_before)
>>> -      << PP.getSpelling(Tok);
>>> -    return 0;
>>> -  }
>>> -  ConsumeToken();
>>> +  if (Tok.is(tok::kw_typename) || Tok.is(tok::kw_struct)) {
>>> +    Diag(Tok.getLocation(), diag::err_expected_class_instead)
>>> +      << PP.getSpelling(Tok)
>>> +      << FixItHint::CreateReplacement(Tok.getLocation(), "class");
>>> +    ConsumeToken();
>>> +  } else if (!Tok.is(tok::kw_class))
>>> +      Diag(Tok.getLocation(), diag::err_expected_class_before)
>>> +        << PP.getSpelling(Tok)
>>> +        << FixItHint::CreateInsertion(Tok.getLocation(), "class ");
>> 
>> This seems overly eager in providing a Fix-It.
> 
> Possibly - I still don't have a clear intuition about how this is gauged.
> 
>> Shouldn't we check that the next token is something that actually makes sense for a template template parameter, e.g., '…', an identifier, a ',', '>', or '>>'?
> 
> I'm not sure this is necessary, though. The previous tokens seem like
> it'd be relatively unambiguous that someone was trying to write a
> template template parameter regardless of further errors they may've
> written. I suppose if they wrote an extra ">" things could a little
> more confused, though.


We shouldn't emit the Fix-It unless it looks like we actually do have a template template parameter, which means checking that the next token is something that makes sense for a template template parameter following the 'class'.

	- Doug