<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></div><br><div><div>On Jul 9, 2014, at 7:00 AM, Aaron Ballman <<a href="mailto:aaron.ballman@gmail.com">aaron.ballman@gmail.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 Tue, Jul 8, 2014 at 10:41 PM, Tyler Nowicki <<a href="mailto:tnowicki@apple.com">tnowicki@apple.com</a>> wrote:<br><blockquote type="cite">Hi,<br><br>Thanks for all the feedback. I’ve applied your suggestions. Please see the attached patch.<br><br>I tried terminating the token list with EoF but it didn’t produce the desired result. I don’t know enough about parsing to even try to say what was happening. It seemed like something about the parsers state was messed up after the eof. Also I tried the example, vectorize_width(1+) 1 and it does seem to consume tokens past the end of the directive. Any ideas how I can fix this?<br><br><blockquote type="cite"><blockquote type="cite">+bool Sema::DiagnoseLoopHintValue(Expr *ValueExpr) {<br></blockquote><br>This should take a const Expr *.<br></blockquote><br><br>Unfortunately VerifyIntegerConstantExpression() for getting the IntAPS of the expression doesn’t take a const Expr *, although I don’t think it modifies the result. So we have to pass in an Expr *.<br></blockquote><br>Ah, yes, and changing VerifyIntegerConstantExpression() would require<br>updating quite a few signatures. That makes sense.<br><br><blockquote type="cite"><br><blockquote type="cite"><blockquote type="cite">+ if (!R.isUsable()) {<br>+ Diag(Args[0].getLocation(), diag::err_pragma_loop_invalid_expression);<br>+ return LoopHint(); // Uninitialized loop hints are ignored.<br>+ }<br>+<br>+ QualType QT = R.get()->getType();<br>+ if (!QT->isIntegerType() || QT->isBooleanType() || QT->isCharType()) {<br></blockquote><br>Are char and bool types truly that heinous? (I agree they would look<br>odd, but do they require restricting?). What about scoped<br>enumerations?<br></blockquote><br>I don’t like the idea of allowing statements like #pragma clang loop vectorize_width(‘A’) because it is unclear what we should do. What is an ‘A’ width? I’ve added a test for scoped enumerations.<br></blockquote><br>Fair.<br><br><blockquote type="cite"><br><blockquote type="cite">+ /// \brief Transform the given attribute.<br>+ ///<br>+ /// By default, this routine transforms a statement by delegating to the<br>+ /// appropriate TransformXXXAttr function to transform a specific kind<br>+ /// of attribute. Subclasses may override this function to transform<br>+ /// attributed statements using some other mechanism.<br>+ ///<br>+ /// \returns the transformed attribute<br>+ const Attr *TransformAttr(const Attr *S);<br><br>I don't like that this gives us two completely different ways to instantiate attributes (TreeTransform::transformAttrs / Sema::InstantiateAttrs and this new function). I'm OK with this as a stopgap measure if you're prepared to clean this duplication up afterwards, but this is not reasonable as a long-term situation.<br></blockquote><br>Each is used for a different kind of attribute. Attributes on declaration and attributes on statements. Each attribute is handled in a completely different manner throughout clang. For example, statement attributes are stored in a wrapper called an AttributedStmt while declaration attributes are stored in a list that is mapped to the pointer of the Decl object. I’m not sure why they are treated differently, but it would be a lot of work to unify them. There are currently only two statement attributes: loop hint and switch fallthrough.<br><br><blockquote type="cite">+ ValueInt = ValueAPS.getSExtValue();<br>+ }<br><br>What if the value doesn't fit in 64 bits? Asserting is not a friendly response. Since you're looking for a 32-bit int anyway, maybe saturate at 0xFFFFF’FFFF?<br></blockquote><br>Not sure what you mean by saturate at 0xFF…? The loop hint metadata only takes a 32-bit value so we can’t support anything more than that. I added a condition on the value that it be represented with at most 32 bits. Otherwise an invalid_argument error will be triggered which uses a special print method to report the erroneous value. There is already a test for it.<br><br>Tyler<br><br></blockquote><br>Btw, I am unable to apply your patch to ToT with svn. I get very odd<br>results. Eg) Sema.h<br><br>bool CheckLoopHintExpr(Expr *E, SourceLocation Loc, bool AllowValueDependent);<br><br> ArrayRef<Expr *> Args,<br> SourceLocation LitEndLoc,<br> TemplateArgumentListInfo *ExplicitTemplateArgs = nullptr);<br><br>~Aaron</div></blockquote></div><br></body></html>