Hi, Thanks for all the feedback. I’ve applied your suggestions. Please see the attached patch.
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?
>> +bool Sema::DiagnoseLoopHintValue(Expr *ValueExpr) {
>
> This should take a const Expr *.
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 *.
>> + if (!R.isUsable()) {
>> + Diag(Args[0].getLocation(), diag::err_pragma_loop_invalid_expression);
>> + return LoopHint(); // Uninitialized loop hints are ignored.
>> + }
>> +
>> + QualType QT = R.get()->getType();
>> + if (!QT->isIntegerType() || QT->isBooleanType() || QT->isCharType()) {
>
> Are char and bool types truly that heinous? (I agree they would look
> odd, but do they require restricting?). What about scoped
> enumerations?
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.
> + /// \brief Transform the given attribute.
> + ///
> + /// By default, this routine transforms a statement by delegating to the
> + /// appropriate TransformXXXAttr function to transform a specific kind
> + /// of attribute. Subclasses may override this function to transform
> + /// attributed statements using some other mechanism.
> + ///
> + /// \returns the transformed attribute
> + const Attr *TransformAttr(const Attr *S);
>
> 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.
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.
> + ValueInt = ValueAPS.getSExtValue();
> + }
>
> 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?
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.
Tyler
pragma_nontypetemplate-svn.patch
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
