On Thu, Jul 24, 2014 at 6:48 PM, Tyler Nowicki <[email protected]> wrote:
> Hi Aaron,
>
> Thanks for the review!
>
> There were a number of conflicts with the work on pragma unroll and nounroll.
> To accommodate these directives I changed the Enabled argument of the loop
> hint attribute to a state argument. The state argument can be default,
> enable, or disable. Attributes like ‘#pragma unroll’ that don’t have any
> arguments use the default state.
I would prefer this to be a separate change if at all possible (as a
predecessor to this patch) as it seems to be fairly substantive --
there's about a 17kb difference between this patch and the last one.
;-)
>
> I also applied most of your suggestions except for those below. Please review
> the updated patch.
>
>
>>> + ConsumeToken(); // The annotation token.
>>
>> I would hoist this out of the if statement and place it closer to the
>> assert at the top of the method.
> (see next)
>
>>> + // Enter constant expression including eof terminator into token
>>> stream.
>>> + PP.EnterTokenStream(Args, ArgSize, /*DisableMacroExpansion=*/false,
>>> + /*OwnsTokens=*/false);
>>> +
>>> + ConsumeToken(); // The annotation token.
>>
>> This winds up being hoisted and so can be removed.
>
> Moving a PP.Lex before the PP.EnterTokenStream means that when
> ParseConstantExpression() is called Tok is set to the token that follows the
> annotation token, but thats not what we want. What want Tok to be the first
> token in the constant expression. In other words
>
> ConsumeToken
> PP.EnterTokenStream
>
> isn’t the same as
>
> PP.EnterTokenStream
> ConsumeToken
>
> So we can’t hoist ConsumeToken to the top of the function.
Ahh, okay, that makes sense. Thank you for the explanation!
>
>
>>> +
>>> + ExprResult R = ParseConstantExpression();
>>> +
>>> + ConsumeToken(); // The constant expression eof terminator.
>>
>> You should not be assuming this is an eof terminator -- since this is
>> parser functionality, I think using ExpectAndConsume would make the
>> most sense (so you can diagnose if the token isn't present, and fail
>> out properly).
>>
>> A parsing tese where this fails would be good as well.
>
> The terminator is added after the pragma is parsed. I’m not sure how to write
> a test that makes ParseConstantExpression() fail to stop at the terminator.
> The terminator (eof) is specifically added to make sure
> ParseConstantExpression() doesn’t parse past the end of the directive.
Then I would add an assert ensuring that you really have consumed the
EOF and not something else.
> What can go wrong here is ParseConstantExpression() can stop before it has
> consumed all of the tokens in the constant expression. Because the expression
> was ill-formed something like '(1 2)’. It will parse ‘1’ and leave ‘2’ on the
> token stream. The pragma unroll test ‘#pragma unroll 1 2’ tests exactly this.
> I modified the code here to emit a warning and consume all remaining tokens
> in the constant expression.
Ah, great!
>
>
>>> void EmitClangAttrList(RecordKeeper &Records, raw_ostream &OS) {
>>> emitSourceFileHeader("List of all attributes that Clang recognizes", OS);
>>> @@ -1633,14 +1641,25 @@
>>> " INHERITABLE_PARAM_ATTR(NAME)\n";
>>> OS << "#endif\n\n";
>>>
>>> + OS << "#ifndef PRAGMA_SPELLING_ATTR\n";
>>> + OS << "#define PRAGMA_SPELLING_ATTR(NAME)\n";
>>> + OS << "#endif\n\n";
>>> +
>>> + OS << "#ifndef LAST_PRAGMA_SPELLING_ATTR\n";
>>> + OS << "#define LAST_PRAGMA_SPELLING_ATTR(NAME)
>>> PRAGMA_SPELLING_ATTR(NAME)\n";
>>> + OS << "#endif\n\n";
>>> +
>>> Record *InhClass = Records.getClass("InheritableAttr");
>>> Record *InhParamClass = Records.getClass("InheritableParamAttr");
>>> - std::vector<Record*> Attrs = Records.getAllDerivedDefinitions("Attr"),
>>> - NonInhAttrs, InhAttrs, InhParamAttrs;
>>> + std::vector<Record *> Attrs = Records.getAllDerivedDefinitions("Attr"),
>>> + NonInhAttrs, InhAttrs, InhParamAttrs, PragmaAttrs;
>>> for (auto *Attr : Attrs) {
>>> if (!Attr->getValueAsBit("ASTNode"))
>>> continue;
>>> -
>>> +
>>> + if (AttrHasPragmaSpelling(Attr))
>>> + PragmaAttrs.push_back(Attr);
>>> +
>>
>> I think this is fine for now, but I am starting to get worried about
>> the maintainability of this list. It used to be about type
>> categorization, now it's a bit larger. But then again, the type
>> categorization wasn't always perfect either (for instance, something
>> can be a type attribute and an inheritable attribute at the same
>> time); we just sort of skirted around it.
>>
>> So at some point, this code may need to be revisited, but it seems
>> correct right now.
>
> Ok, thats great. The new list is used by sema during template instantiation.
> Rather than hard-coding the attributes that require a special template
> instantiation this list is used to automatically generate transform function
> stubs and their uses. Doing this for every attribute, not just attributes
> with pragma spellings, would be bad because most attributes can only be
> applied to declarations which are handled in a completely different way. So
> it would create many useless stub functions.
Good to know.
~Aaron
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
