r198605 - Don't use magic constants in the digraph diagnostic

[Alp Toker]

On 06/01/2014 17:34, Jordan Rose wrote:
> This changes the message for templates in a way that is less informative.

No change in output. Do you mean the string in the TableGen file is now 
less informative?

> In other cases where we use tokens as diagnostic arguments, the message makes sense whether or not the token text is quoted. Here, that's not true: the problem isn't about the literal word "template" but about a particular template (like "vector"). Using tok::annot_template_id as if it were a real token seems wrong

That was the reason I changed the "magic token kind" in use from 
tok::kw_template to tok::annot_template_id. tok::kw_template relates to 
"template" whereas tok::annot_template_id is a synonym for "template 
name" so this appears a better choice to use as the placeholder.

The thought experiment here is that 
ExpectAndConsume(tok::annot_template_id) would emit the correct 
diagnostic message, which it hopefully does at this point (other than 
that it would assert right now due to the special token check).

> , particularly because the comment says it's intended for use with function template names.

That's an interesting point but I didn't quite understand. Can you 
explain a bit further?

>
> I'd rather have two variants for this diagnostic, or use an enum specifically marked as "keep in sync with this diagnostic" (slightly better than 0-1-2-3-4). Is there a reason you specifically wanted to change this one?

Chose this diagnostic to demonstrate the new diagnostic printing 
facility introduced in the previous commit. It looked like a good 
candidate to upgrade first because of the odd use of tok::kw_template.

The idea with the ongoing diagnostic effort is to make the messages more 
consistent, clearly written and easy to maintain so if it's missing the 
mark we should deal with that.

Alp.


>
> Jordan
>
>
>
> On Jan 6, 2014, at 4:54 , Alp Toker <alp at nuanti.com> wrote:
>
>> Author: alp
>> Date: Mon Jan  6 06:54:32 2014
>> New Revision: 198605
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=198605&view=rev
>> Log:
>> Don't use magic constants in the digraph diagnostic
>>
>> Modified:
>>     cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td
>>     cfe/trunk/lib/Parse/ParseExprCXX.cpp
>>
>> Modified: cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td?rev=198605&r1=198604&r2=198605&view=diff
>> ==============================================================================
>> --- cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td (original)
>> +++ cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td Mon Jan  6 06:54:32 2014
>> @@ -641,9 +641,7 @@ def err_ctor_init_missing_comma : Error<
>> def err_friend_decl_defines_type : Error<
>>    "cannot define a type in a friend declaration">;
>> def err_missing_whitespace_digraph : Error<
>> -  "found '<::' after a "
>> -  "%select{template name|const_cast|dynamic_cast|reinterpret_cast|static_cast}0"
>> -  " which forms the digraph '<:' (aka '[') and a ':', did you mean '< ::'?">;
>> +  "found '<::' after a %0 which forms the digraph '<:' (aka '[') and a ':', did you mean '< ::'?">;
>>
>> def ext_deleted_function : ExtWarn<
>>    "deleted function definitions are a C++11 extension">, InGroup<CXX11>;
>>
>> Modified: cfe/trunk/lib/Parse/ParseExprCXX.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/ParseExprCXX.cpp?rev=198605&r1=198604&r2=198605&view=diff
>> ==============================================================================
>> --- cfe/trunk/lib/Parse/ParseExprCXX.cpp (original)
>> +++ cfe/trunk/lib/Parse/ParseExprCXX.cpp Mon Jan  6 06:54:32 2014
>> @@ -24,18 +24,6 @@
>>
>> using namespace clang;
>>
>> -static int SelectDigraphErrorMessage(tok::TokenKind Kind) {
>> -  switch (Kind) {
>> -    case tok::kw_template:         return 0;
>> -    case tok::kw_const_cast:       return 1;
>> -    case tok::kw_dynamic_cast:     return 2;
>> -    case tok::kw_reinterpret_cast: return 3;
>> -    case tok::kw_static_cast:      return 4;
>> -    default:
>> -      llvm_unreachable("Unknown type for digraph error message.");
>> -  }
>> -}
>> -
>> // Are the two tokens adjacent in the same source file?
>> bool Parser::areTokensAdjacent(const Token &First, const Token &Second) {
>>    SourceManager &SM = PP.getSourceManager();
>> @@ -56,8 +44,7 @@ static void FixDigraph(Parser &P, Prepro
>>    Range.setBegin(DigraphToken.getLocation());
>>    Range.setEnd(ColonToken.getLocation());
>>    P.Diag(DigraphToken.getLocation(), diag::err_missing_whitespace_digraph)
>> -      << SelectDigraphErrorMessage(Kind)
>> -      << FixItHint::CreateReplacement(Range, "< ::");
>> +      << Kind << FixItHint::CreateReplacement(Range, "< ::");
>>
>>    // Update token information to reflect their change in token type.
>>    ColonToken.setKind(tok::coloncolon);
>> @@ -93,8 +80,8 @@ void Parser::CheckForTemplateAndDigraph(
>>                                Template, MemberOfUnknownSpecialization))
>>      return;
>>
>> -  FixDigraph(*this, PP, Next, SecondToken, tok::kw_template,
>> -             /*AtDigraph*/false);
>> +  FixDigraph(*this, PP, Next, SecondToken, tok::annot_template_id,
>> +             /*AtDigraph*/ false);
>> }
>>
>> /// \brief Emits an error for a left parentheses after a double colon.
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

-- 
http://www.nuanti.com
the browser experts