<div dir="ltr"><div>+++ b/lib/Parse/ParsePragma.cpp<br></div><div>[...]</div><div>+ if (R.isInvalid() ||</div><div>+ Actions.CheckLoopHintExpr(R.get(), Toks[0].getLocation()))</div><div><br></div><div>More indentation here please.</div><div><br></div><div><div>+ SmallVector<Token, 1> ValueList;</div><div>+ int OpenParens = ValueInParens ? 1 : 0;</div><div>+ // Read constant expression.</div><div>+ while (Tok.isNot(tok::eod)) {</div><div>+ if (Tok.is(tok::l_paren))</div><div>+ OpenParens++;</div><div>+<span class="" style="white-space:pre"> </span>else if (Tok.is(tok::r_paren)) {</div><div>+ OpenParens--;</div><div>+<span class="" style="white-space:pre"> </span> if(OpenParens == 0 && ValueInParens)</div><div>+<span class="" style="white-space:pre"> </span> break;</div><div> }</div><div> <br></div><div>+<span class="" style="white-space:pre"> </span>ValueList.push_back(Tok);<br></div><div>+<span class="" style="white-space:pre"> </span>PP.Lex(Tok);</div><div>+ }</div></div><div><br></div><div>Some hard tab characters have snuck in here; please fix.</div><div><br></div><div>+ Token EoF;<br></div><div><br></div><div>OK, if we can't call this EOF then either EofTok or EOFTok? =)</div><div><br></div><div><br></div><div>+++ b/lib/Sema/SemaExpr.cpp<br></div><div><div>+ if (E->isValueDependent()) {</div><div>+ return false;</div><div>+ }</div></div><div><br></div><div>No braces here, please.</div><div><br></div><div><div>+ if (!ValueAPS.isStrictlyPositive() || ValueAPS.getBitWidth() > 32) {</div><div>+<span class="" style="white-space:pre"> </span>Diag(E->getExprLoc(), diag::err_pragma_loop_invalid_argument_value) << ValueAPS.toString(10);</div><div>+ return true;</div><div>+ }</div></div><div><br></div><div>I don't see why we need to care about the width of the type here; it would seem better to simply require that the value fits into 31 bits, if that's the actual requirement imposed by the IR representation. That is, relpace the getBitWidth() check with 'ValueAPS.getActiveBits() <= 31'. (As an extreme example, consider a target where *all* integer types are 64 bits wide.)</div><div><br></div><div>+++ b/test/CodeGen/pragma-loop.cpp<br></div><div><div>+// Test for scoped enumerations.</div><div>+enum Tuner { Interleave = 4,</div><div>+ Unroll = 8 };</div></div><div><br></div><div>This is an unscoped enumeration.</div><div><br></div><div>+++ b/include/clang/Basic/DiagnosticParseKinds.td<br></div><div><div>+def err_pragma_loop_missing_argument : Error<</div><div>+ "missing argument; expected %select{a positive 32-bit integer value|"</div></div><div><br></div><div>This "expected a positive 32-bit integer value" wording seems like the wrong thing to be saying here, because it doesn't actually tell the user what they missed out. It'd be much friendlier to say "expected loop unroll count" or similar. Also, the sign and bit width information is a distraction from the message.<br></div><div><br></div><div><div>+def err_pragma_loop_invalid_argument_type : Error<</div><div>+ "invalid argument %0; expected a 32-bit integer value">;</div></div><div><br></div><div>You should specifically say that the type is wrong here. Something like "invalid argument of type %0; loop %select{unroll|interleave}1 count should have integer type" would be better.</div><div><br></div><div><div>+def err_pragma_loop_invalid_argument_value : Error<</div><div>+ "invalid argument '%0'; expected a positive 32-bit integer value">;</div></div><div><br></div><div>Arguably 0x80000000u is a positive 32-bit integer value, but you reject it. Something like "invalid argument %0; loop %select{unroll|interleave}1 count must be %select{positive|%3 or smaller}2" would be nice.</div><div><br></div><div><div>+def err_pragma_loop_invalid_expression : Error<</div><div>+ "invalid expression; expected an integer constant expression">;</div></div><div><br></div><div>This adds no information to the normal "not an ICE" diagnostic. Either rewrite this to be friendlier ("loop unroll count is not an integral constant expression") or just remove this and use the default diagnostic.</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Sep 15, 2014 at 1:12 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">Ping... x2</div><div class="HOEnZb"><div class="h5"><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Sep 11, 2014 at 2:38 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">Ping...<div><div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Sep 5, 2014 at 1:30 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"><div class="gmail_extra">Hi Richard,</div><div class="gmail_extra"><br></div><div class="gmail_extra">Thanks for the review. Sorry for the delayed response. I made the changes you suggested except for the following.</div><span><div class="gmail_extra"><br></div><div class="gmail_extra"><blockquote style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex" class="gmail_quote">+ Token EoF;<br>The 'o' in EOF is traditionally capitalized.</blockquote></div><div class="gmail_extra"><br></div></span><div class="gmail_extra">Unfortunately EOF is defined by stdio.h and at least in VS the conflict triggers a compilation error. Also in PragmaMSPragma::HandlePragma EoF is used.</div><div class="gmail_extra"><div class="gmail_quote"><div><br></div><div>Please review the attached patch.</div><div><br></div><div>Thanks,</div><div><br></div><div>Tyler</div><div><br></div><div><br></div></div></div></div>
</blockquote></div><br></div></div></div></div>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div>