r198605 - Don't use magic constants in the digraph diagnostic
Alp Toker
alp at nuanti.com
Mon Jan 6 18:14:47 PST 2014
On 06/01/2014 19:11, Richard Smith wrote:
> On Mon Jan 06 2014 at 11:01:02 AM, Alp Toker <alp at nuanti.com
> <mailto:alp at nuanti.com>> wrote:
>
>
> On 06/01/2014 18:29, Richard Smith wrote:
> > On Mon Jan 06 2014 at 10:06:25 AM, Alp Toker <alp at nuanti.com
> <mailto:alp at nuanti.com>
> > <mailto:alp at nuanti.com <mailto: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.
>
> Diagnostics seem to call it that way, with 'template-id' not appearing
> rarely:
>
> def err_template_id_not_a_type : Error<
> "template name refers to non-type template %0">;
>
>
> This looks clear and technically correct to me. We're diagnosing that
> the template-id X<Y> is not a type by saying that X is a non-type
> template name.
>
> I agree it's not fully appropriate, but it's more appropriate than the
> previous tok::kw_template given that there isn't a "template"
> keyword in
> sight:
>
> D<::F> A2; // expected-error{{found '<::' after a template name which
> forms the digraph}}
>
> Can you think of a better alternative to either?
>
>
> Jordan's suggestion of using an enum seems fine to me. (As Aaron
> Ballman and I have previously discussed on-list, we should also have a
> mechanism to generate these enums from the .td files to avoid the
> magic numbers entirely.)
Even with an enum there's still going to have to be a switch somewhere,
and what I've really been looking for is boilerplate-free way to share
code construct descriptions with the parser, sema and code completion.
Agree that this change might not be the solution (especially
annot_type_id) but the previous code was at least as dubious with its
kw_template so I'm not keen on bringing that back to life either.
So if it's OK with Jordan I'd like to mull over this for a few days to
see if we can move it forward in a satisfactory way first. Will examine
those threads again too.
Alp.
>
> Alp.
>
>
> > 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
> <mailto:alp at nuanti.com>
> > <mailto:alp at nuanti.com <mailto: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 <mailto:cfe-commits at cs.uiuc.edu>
> <mailto:cfe-commits at cs.uiuc.edu <mailto: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 <mailto:cfe-commits at cs.uiuc.edu>
> <mailto:cfe-commits at cs.uiuc.edu <mailto:cfe-commits at cs.uiuc.edu>>
> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> >
>
> --
> http://www.nuanti.com
> the browser experts
>
--
http://www.nuanti.com
the browser experts
More information about the cfe-commits
mailing list