<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Sep 24, 2014 at 12:11 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:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>Hi Richard,</div><div><br></div><div>Thanks for the review. Sorry for the delayed response. I applied most of your suggestions. I have some comments on your suggestions for the diagnostic messages. Please review the attached patch.</div><span class=""><div> </div><blockquote class="gmail_quote" 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">+++ b/include/clang/Basic/DiagnosticParseKinds.td<br></blockquote><blockquote class="gmail_quote" 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"><div dir="ltr"><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.</div></div></blockquote><div><br></div></span><div>I went with "expected an integer value". The loop hint option, such as vectorize_width is already indicated by the error message with a caret pointing to it. Including more information makes the error message kind of redundant. Let me know what you think.</div></div></div></div></blockquote><div><br></div><div>OK, I agree the meaning of the integer is sufficiently obvious from the preceding identifier.</div><div> </div><blockquote class="gmail_quote" 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"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class=""><blockquote class="gmail_quote" 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"><div dir="ltr"><div></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></blockquote><div><br></div></span><div>I went with "expected an integer type". See my previous rational.</div><span class=""><div><br></div><div> </div><blockquote class="gmail_quote" 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"><div dir="ltr"><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></blockquote><div><br></div></span><div>We cannot know what the maximums are at this point. Fortunately, the optimization passes will generate error messages when the loop directive cannot be followed. For example, an error is produce when the specified vector width is too large. So we just need to make sure the input fits into the IR. But we also shouldn't mislead the programmer with incorrect maximums. For example, 'loop unroll must be 1000000 or smaller' is a poor error because that maximum probably won't work.</div><div><br></div><div>I went with 'invalid value '%0'; expected a positive 32-bit signed integer value'.</div></div></div></div></blockquote><div><br></div><div>This still seems awkward to me. If you can't give an upper bound, then just saying that it's too large would seem fine. 'invalid value %0; must be positive' and 'value %0 is too large' or similar?</div><div><br></div><div><div>+  Token *TokenArray = (Token *)PP.getPreprocessorAllocator().Allocate(</div><div>+      ValueList.size() * sizeof(Token), llvm::alignOf<Token>());</div></div><div><br></div><div>Can you use</div><div><br></div><div>  Token *TokenArray = new (PP.getPreprocessorAllocator()) Token[ValueList.size()];</div><div><br></div><div>here?</div><div><br></div><div>Anyway... LGTM. I'm fine with improving the "positive 32-bit signed integer value" wording post-commit if you'd prefer.</div></div></div></div>