On 06/01/2014 19:11, Richard Smith wrote:
On Mon Jan 06 2014 at 11:01:02 AM, Alp Toker <[email protected] <mailto:[email protected]>> wrote:On 06/01/2014 18:29, Richard Smith wrote: > On Mon Jan 06 2014 at 10:06:25 AM, Alp Toker <[email protected] <mailto:[email protected]> > <mailto:[email protected] <mailto:[email protected]>>> 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 <[email protected] <mailto:[email protected]> > <mailto:[email protected] <mailto:[email protected]>>> 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 > >> [email protected] <mailto:[email protected]> <mailto:[email protected] <mailto:[email protected]>> > >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > > -- > http://www.nuanti.com > the browser experts > > _______________________________________________ > cfe-commits mailing list > [email protected] <mailto:[email protected]> <mailto:[email protected] <mailto:[email protected]>> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > -- http://www.nuanti.com the browser experts
-- http://www.nuanti.com the browser experts _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
