<div dir="ltr"><div>+ // Tokens following an error in an ill-formed constant expression will<br></div><div>+ // remain in the token stream and must be removed.</div><div>+ if (Tok.isNot(tok::eof)) {</div><div>+ Diag(Tok.getLocation(), diag::warn_pragma_extra_tokens_at_eol)</div>
<div>+ << PragmaLoopHintString(Info->PragmaName, Info->Option);</div><div>+ while (Tok.isNot(tok::eof))</div><div>+ ConsumeToken();</div><div><br></div><div>You need to use ConsumeAnyToken() here, or this will fail if it hits certain kinds of special tokens.</div>
<div><br></div><div><div>+ if (Actions.CheckLoopHintExpr(R.get(), Toks[0].getLocation(),</div><div>+ /*AllowValueDependent=*/true))</div><div>+ return false;</div></div><div><br></div>
<div>If R.isInvalid(), you should bail out here rather than calling Sema with a null pointer.</div><div><br></div><div><div>+ // Read constant expression.</div><div>+ // FIXME: This loop stops on first ')'. Should use BalancedDelimiterTracker</div>
<div>+ // to track parens and parse expressions like ((i+1)*2).</div></div><div><br></div><div>BalancedDelimiterTracker doesn't help here; this is not what it's for. You can just keep a simple count of the number of unclosed parens you've encountered.</div>
<div><br></div><div>+ Token EoF;<br></div><div><br></div><div>The 'o' in EOF is traditionally capitalized.</div><div><br></div><div><div>+ if (!E) {</div><div>+ Diag(Loc, diag::err_pragma_loop_invalid_expression);</div>
<div>+ return true;</div><div>+ }</div><div><br></div></div><div>This seems wrong: if we couldn't parse an expression, we should have already produced a diagnostic. But I think it's redundant given the change I suggested above.</div>
<div><br></div><div><div>+ if (E->isValueDependent()) {</div><div>+ if (AllowValueDependent)</div><div>+ return false;</div><div>+ Diag(E->getExprLoc(), diag::err_pragma_loop_invalid_expression);</div><div>
+ return true;</div><div>+ }</div></div><div><br></div><div>The AllowValueDependent flag here is unnecessary; instead, always allow value-dependent things. You won't see value-dependent expressions here unless you're still in a template or some other error has already occurred (and been diagnosed).</div>
<div><br></div><div><div>+ if (!R.isUsable())</div><div>+ return true;</div></div><div><br></div><div>Please use isInvalid() here. isUsable suggests that you're anticipating valid-but-null results.</div><div><br></div>
<div>+ std::string Value = "\'" + ValueAPS.toString(10) + "\'";<br></div><div><br></div><div>We usually put the ''s in the diagnostic, not in the argument.</div></div><div class="gmail_extra">
<br><br><div class="gmail_quote">On Mon, Aug 25, 2014 at 9:10 AM, Tyler Nowicki <span dir="ltr"><<a href="mailto:tyler.nowicki@gmail.com" target="_blank">tyler.nowicki@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr">Ping...<div><div class="h5"><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Aug 21, 2014 at 3:32 PM, Tyler Nowicki <span dir="ltr"><<a href="mailto:tyler.nowicki@gmail.com" target="_blank">tyler.nowicki@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Hi,<div><br></div><div>I am back from my post-internship vacation. Aaron has previously given this patch a LGTM, but he suggested Richard take a look as well.</div>
<div><br></div><div>@Richard, could you please review this patch.</div>
<div><br></div><div>In follow-up patches I will move CodeGen/pragma-loop.cpp to CodeGenCXX and use BalancedDelimiterTracker to ensure parens are balanced when parsing the constant expression. Please email me at <a href="mailto:tyler.nowicki@gmail.com" target="_blank">tyler.nowicki@gmail.com</a> (not <a href="mailto:tnowicki@apple.com" target="_blank">tnowicki@apple.com</a>).</div>
<div><br></div><div>Thanks,</div><div><br></div><div>Tyler Nowicki</div></div>
</blockquote></div><br></div></div></div></div>
</blockquote></div><br></div>