<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div>Thanks, I’ll get that commented-out test in-place.</div><div><br></div><div>I look forward to Richard’s review.</div><div><br></div><div>Tyler</div><div><br></div><div><div>On Aug 1, 2014, at 12:10 PM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">On Fri, Aug 1, 2014 at 3:06 PM, Tyler Nowicki <<a href="mailto:tnowicki@apple.com">tnowicki@apple.com</a>> wrote:<br><blockquote type="cite"><blockquote type="cite"><blockquote type="cite">+ SmallVector<Token, 1> ValueList;<br>+ // Read constant expression.<br>+ while (Tok.isNot(tok::r_paren) && Tok.isNot(tok::eod)) {<br>+ ValueList.push_back(Tok);<br>+ PP.Lex(Tok);<br></blockquote><br>I don't think this is correct -- it won't handle constant expressions<br>involving parentheses properly. Eg)<br><br>#pragma unroll((2+2)+4)<br><br>It produces:<br>E:\Aaron Ballman\Desktop\test2.cpp:9:21: warning: extra tokens at end<br>of '#pragma unroll' - ignored<br>#pragma unroll((2+2)+4)<br><br>So that should be fixed, and you should have a test for it as well.<br></blockquote><br>Oops, you are right. How about we do this incrementally? We can put this patch in now and I'll follow up with better parsing. I’ll be able work on this in a couple of weeks.<br><br>I think we need to use the BalancedDelimiterTracker to resolve this issue.<br></blockquote><br>If you put in a FIXME with the code, and a commented-out test case<br>with a FIXME, I can live with that. I also agree that this code should<br>be using the BalancedDelimiterTracker. It will simplify a bit of the<br>code when you switch over to it.<br><br>The rest of the patch LGTM, but I would also wait for Richard to<br>comment on the patch just in case (he's more familiar with the<br>constant expression stuff than I am).<br><br>Also, don't forget to move the test cases to their appropriate directories. ;-)<br><br>~Aaron</div></blockquote></div><br></body></html>