<div>On Mon Jan 06 2014 at 11:01:02 AM, Alp Toker <<a href="mailto:alp@nuanti.com">alp@nuanti.com</a>> wrote:</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
On 06/01/2014 18:29, Richard Smith wrote:<br>
> On Mon Jan 06 2014 at 10:06:25 AM, Alp Toker <<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a><br>
> <mailto:<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>>> wrote:<br>
><br>
><br>
> On 06/01/2014 17:34, Jordan Rose wrote:<br>
> > This changes the message for templates in a way that is less<br>
> informative.<br>
><br>
> No change in output. Do you mean the string in the TableGen file<br>
> is now<br>
> less informative?<br>
><br>
> > In other cases where we use tokens as diagnostic arguments, the<br>
> message makes sense whether or not the token text is quoted. Here,<br>
> that's not true: the problem isn't about the literal word<br>
> "template" but about a particular template (like "vector"). Using<br>
> tok::annot_template_id as if it were a real token seems wrong<br>
><br>
> That was the reason I changed the "magic token kind" in use from<br>
> tok::kw_template to tok::annot_template_id. tok::kw_template<br>
> relates to<br>
> "template" whereas tok::annot_template_id is a synonym for "template<br>
> name" so this appears a better choice to use as the placeholder.<br>
><br>
><br>
> It's a synonym for 'template id' (a template name followed by template<br>
> arguments), not for 'template name'. Using it here is inappropriate.<br>
<br>
Diagnostics seem to call it that way, with 'template-id' not appearing<br>
rarely:<br>
<br>
def err_template_id_not_a_type : Error<<br>
"template name refers to non-type template %0">;<br></blockquote><div><br></div><div>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.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I agree it's not fully appropriate, but it's more appropriate than the<br>
previous tok::kw_template given that there isn't a "template" keyword in<br>
sight:<br>
<br>
D<::F> A2; // expected-error{{found '<::' after a template name which<br>
forms the digraph}}<br>
<br>
Can you think of a better alternative to either?<br></blockquote><div><br></div><div>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.)</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Alp.<br>
<br>
<br>
> The thought experiment here is that<br>
> ExpectAndConsume(tok::annot_<u></u>template_id) would emit the correct<br>
> diagnostic message, which it hopefully does at this point (other than<br>
> that it would assert right now due to the special token check).<br>
><br>
> > , particularly because the comment says it's intended for use<br>
> with function template names.<br>
><br>
> That's an interesting point but I didn't quite understand. Can you<br>
> explain a bit further?<br>
><br>
> ><br>
> > I'd rather have two variants for this diagnostic, or use an enum<br>
> specifically marked as "keep in sync with this diagnostic"<br>
> (slightly better than 0-1-2-3-4). Is there a reason you<br>
> specifically wanted to change this one?<br>
><br>
> Chose this diagnostic to demonstrate the new diagnostic printing<br>
> facility introduced in the previous commit. It looked like a good<br>
> candidate to upgrade first because of the odd use of tok::kw_template.<br>
><br>
> The idea with the ongoing diagnostic effort is to make the<br>
> messages more<br>
> consistent, clearly written and easy to maintain so if it's<br>
> missing the<br>
> mark we should deal with that.<br>
><br>
> Alp.<br>
><br>
><br>
> ><br>
> > Jordan<br>
> ><br>
> ><br>
> ><br>
> > On Jan 6, 2014, at 4:54 , Alp Toker <<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a><br>
> <mailto:<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>>> wrote:<br>
> ><br>
> >> Author: alp<br>
> >> Date: Mon Jan 6 06:54:32 2014<br>
> >> New Revision: 198605<br>
> >><br>
> >> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=198605&view=rev" target="_blank">http://llvm.org/viewvc/llvm-<u></u>project?rev=198605&view=rev</a><br>
> >> Log:<br>
> >> Don't use magic constants in the digraph diagnostic<br>
> >><br>
> >> Modified:<br>
> >> cfe/trunk/include/clang/Basic/<u></u>DiagnosticParseKinds.td<br>
> >> cfe/trunk/lib/Parse/<u></u>ParseExprCXX.cpp<br>
> >><br>
> >> Modified: cfe/trunk/include/clang/Basic/<u></u>DiagnosticParseKinds.td<br>
> >> URL:<br>
> <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td?rev=198605&r1=198604&r2=198605&view=diff" target="_blank">http://llvm.org/viewvc/llvm-<u></u>project/cfe/trunk/include/<u></u>clang/Basic/<u></u>DiagnosticParseKinds.td?rev=<u></u>198605&r1=198604&r2=198605&<u></u>view=diff</a><br>
> >><br>
> ==============================<u></u>==============================<u></u>==================<br>
> >> --- cfe/trunk/include/clang/Basic/<u></u>DiagnosticParseKinds.td<br>
> (original)<br>
> >> +++ cfe/trunk/include/clang/Basic/<u></u>DiagnosticParseKinds.td Mon<br>
> Jan 6 06:54:32 2014<br>
> >> @@ -641,9 +641,7 @@ def err_ctor_init_missing_comma : Error<<br>
> >> def err_friend_decl_defines_type : Error<<br>
> >> "cannot define a type in a friend declaration">;<br>
> >> def err_missing_whitespace_digraph : Error<<br>
> >> - "found '<::' after a "<br>
> >> - "%select{template<br>
> name|const_cast|dynamic_cast|<u></u>reinterpret_cast|static_cast}<u></u>0"<br>
> >> - " which forms the digraph '<:' (aka '[') and a ':', did you<br>
> mean '< ::'?">;<br>
> >> + "found '<::' after a %0 which forms the digraph '<:' (aka<br>
> '[') and a ':', did you mean '< ::'?">;<br>
> >><br>
> >> def ext_deleted_function : ExtWarn<<br>
> >> "deleted function definitions are a C++11 extension">,<br>
> InGroup<CXX11>;<br>
> >><br>
> >> Modified: cfe/trunk/lib/Parse/<u></u>ParseExprCXX.cpp<br>
> >> URL:<br>
> <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/ParseExprCXX.cpp?rev=198605&r1=198604&r2=198605&view=diff" target="_blank">http://llvm.org/viewvc/llvm-<u></u>project/cfe/trunk/lib/Parse/<u></u>ParseExprCXX.cpp?rev=198605&<u></u>r1=198604&r2=198605&view=diff</a><br>
> >><br>
> ==============================<u></u>==============================<u></u>==================<br>
> >> --- cfe/trunk/lib/Parse/<u></u>ParseExprCXX.cpp (original)<br>
> >> +++ cfe/trunk/lib/Parse/<u></u>ParseExprCXX.cpp Mon Jan 6 06:54:32 2014<br>
> >> @@ -24,18 +24,6 @@<br>
> >><br>
> >> using namespace clang;<br>
> >><br>
> >> -static int SelectDigraphErrorMessage(tok:<u></u>:TokenKind Kind) {<br>
> >> - switch (Kind) {<br>
> >> - case tok::kw_template: return 0;<br>
> >> - case tok::kw_const_cast: return 1;<br>
> >> - case tok::kw_dynamic_cast: return 2;<br>
> >> - case tok::kw_reinterpret_cast: return 3;<br>
> >> - case tok::kw_static_cast: return 4;<br>
> >> - default:<br>
> >> - llvm_unreachable("Unknown type for digraph error message.");<br>
> >> - }<br>
> >> -}<br>
> >> -<br>
> >> // Are the two tokens adjacent in the same source file?<br>
> >> bool Parser::areTokensAdjacent(<u></u>const Token &First, const Token<br>
> &Second) {<br>
> >> SourceManager &SM = PP.getSourceManager();<br>
> >> @@ -56,8 +44,7 @@ static void FixDigraph(Parser &P, Prepro<br>
> >> Range.setBegin(DigraphToken.<u></u>getLocation());<br>
> >> Range.setEnd(ColonToken.<u></u>getLocation());<br>
> >> P.Diag(DigraphToken.<u></u>getLocation(),<br>
> diag::err_missing_whitespace_<u></u>digraph)<br>
> >> - << SelectDigraphErrorMessage(<u></u>Kind)<br>
> >> - << FixItHint::CreateReplacement(<u></u>Range, "< ::");<br>
> >> + << Kind << FixItHint::CreateReplacement(<u></u>Range, "< ::");<br>
> >><br>
> >> // Update token information to reflect their change in token<br>
> type.<br>
> >> ColonToken.setKind(tok::<u></u>coloncolon);<br>
> >> @@ -93,8 +80,8 @@ void Parser::<u></u>CheckForTemplateAndDigraph(<br>
> >> Template,<br>
> MemberOfUnknownSpecialization)<u></u>)<br>
> >> return;<br>
> >><br>
> >> - FixDigraph(*this, PP, Next, SecondToken, tok::kw_template,<br>
> >> - /*AtDigraph*/false);<br>
> >> + FixDigraph(*this, PP, Next, SecondToken, tok::annot_template_id,<br>
> >> + /*AtDigraph*/ false);<br>
> >> }<br>
> >><br>
> >> /// \brief Emits an error for a left parentheses after a double<br>
> colon.<br>
> >><br>
> >><br>
> >> ______________________________<u></u>_________________<br>
> >> cfe-commits mailing list<br>
> >> <a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a> <mailto:<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.<u></u>edu</a>><br>
> >> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/<u></u>mailman/listinfo/cfe-commits</a><br>
><br>
> --<br>
> <a href="http://www.nuanti.com" target="_blank">http://www.nuanti.com</a><br>
> the browser experts<br>
><br>
> ______________________________<u></u>_________________<br>
> cfe-commits mailing list<br>
> <a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a> <mailto:<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.<u></u>edu</a>><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/<u></u>mailman/listinfo/cfe-commits</a><br>
><br>
<br>
--<br>
<a href="http://www.nuanti.com" target="_blank">http://www.nuanti.com</a><br>
the browser experts<br>
<br>
</blockquote>