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

[Richard Smith]

On Mon Jan 06 2014 at 10:06:25 AM, Alp Toker <alp at nuanti.com> wrote:

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

It's a synonym for 'template id' (a template name followed by template
arguments), not for 'template name'. Using it here is inappropriate.


> 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
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140106/f3126383/attachment.html>