On Tue, Jul 8, 2014 at 10:41 PM, Tyler Nowicki <[email protected]> wrote:
> 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 *.
Ah, yes, and changing VerifyIntegerConstantExpression() would require
updating quite a few signatures. That makes sense.
>
>>> + 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.
Fair.
>
>> + /// \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
>
Btw, I am unable to apply your patch to ToT with svn. I get very odd
results. Eg) Sema.h
bool CheckLoopHintExpr(Expr *E, SourceLocation Loc, bool AllowValueDependent);
ArrayRef<Expr *> Args,
SourceLocation LitEndLoc,
TemplateArgumentListInfo *ExplicitTemplateArgs = nullptr);
~Aaron
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
