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 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.
>> +
>> + 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. 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.
>> 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.
Tyler
pragma_constantexpression-svn.patch
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
