Thanks! I have some inline comments below.<br><br>On Wed, Jan 16, 2013 at 2:27 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br>
<div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Index: utils/TableGen/ClangAttrEmitter.cpp<br>
===================================================================<br>
--- utils/TableGen/ClangAttrEmitter.cpp (revision 172543)<br>
+++ utils/TableGen/ClangAttrEmitter.cpp (working copy)<br>
@@ -735,6 +735,82 @@<br>
[...]<br>
+ OS <<<br>
+ " case(" << I << ") : {\n"<br>
+ " OS << \"" + Prefix.str() + Spelling.str();<br>
+<br>
+ if (Args.size()) OS << "(";<br>
<br>
Please only generate parens if there are arguments. This has a<br>
semantic impact; for instance, [[noreturn()]] is ill-formed in C++11. <br></blockquote><div><br>This code will not emit the parens if there is no arguments as Args.size() will be zero thus OS << "(" will not be executed. Is there an issue here?<br>
<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
@@ -1044,6 +1105,63 @@<br>
[...]<br>
+ // We only care about attributes that participate sema checking, so<br>
+ // skip those attributes that are not able to make their way to Sema.<br>
<br>
... participate *in* Sema checking<br>
<br>
+ if (!R.getValueAsBit("SemaHandler"))<br>
+ continue;<br>
<br>
Is this check necessary?<br></blockquote><div><br>I think it is necessary. Attributes with sema handler set to 0 never made to Sema, and as a result no Attr will be constructed for these attributes.<br> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
+ // Each distinct spelling yield an attribute kind.<br>
+ if (R.getValueAsBit("DistinctSpellings")) {<br>
+ for (unsigned I = 0; I < Spellings.size(); ++ I) {<br>
+ OS <<<br>
+ " case AT_" << Spellings[I]->getValueAsString("Name") << ": {\n"<br>
+ " Index = " << I << ";\n"<br>
+ " break;\n"<br>
+ "}\n";<br>
<br>
I think all DistinctSpellings spellings should have spelling index 0.<br>
<br></blockquote><div> </div><div>There are many parsed attribute kinds for an attribute with DistinctSpellings set to 1, but it is still a single parsed attribute (and I think in fact we only have one attribute with DistinctSpellings set to 1 in clang : the OwnershipAttr) and share a single spelling list that contain multiple spellings, with each spelling maps to a source form, so we need different indexes here.<br>
<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ OS << " if (Name == \""<br>
+ << Spellings[I]->getValueAsString("Name") << "\" && "<br>
+ << "AttrSyntax == \"" << Spellings[I]->getValueAsString("Variety")<br>
+ << "\" && Scope == \"" << Namespace << "\")\n"<br>
+ << " return " << I << ";\n";<br>
<br>
Please convert the Variety into the syntax enumeration, rather than<br>
converting the syntax value into a string, and compare this first.<br>
It'd be great to use a StringSwitch here, too (or even something<br>
faster, since we know that one of the syntaxes must match).<br>
<br></blockquote><div><br>Thanks, I'll use the enumeration for now and think about produce faster code here.<br> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Index: lib/Sema/AttributeList.cpp<br>
===================================================================<br>
--- lib/Sema/AttributeList.cpp (revision 172543)<br>
+++ lib/Sema/AttributeList.cpp (working copy)<br>
@@ -125,3 +125,39 @@<br>
<br>
return ::getAttrKind(Buf);<br>
}<br>
+<br>
+unsigned AttributeList::getAttributeSpellListIndex() const {<br>
<br>
getAttributeSpellingListIndex?<br>
<br>
+ switch (SyntaxUsed) {<br>
+ default : {<br>
+ llvm_unreachable("Unknown attribute syntax!");<br>
+ break;<br>
+ }<br>
+ case (AS_GNU) : {<br>
+ AttrSyntax = "GNU";<br>
+ break;<br>
+ }<br>
<br>
This should be formatted as:<br></blockquote><div><br>Thanks for pointing this out! I'll kill the curly braces in tablegen emitted code too :) <br><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
switch (SyntaxUsed) {<br>
default:<br>
llvm_unreachable("Unknown attribute syntax!");<br>
<br>
case AS_GNU:<br>
AttrSyntax = "GNU";<br>
break;<br>
<br>
... but, as noted above, don't do this conversion.<br>
<div><div><br></div></div></blockquote><div><br>Thanks!<br>Michael<br> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div>
On Wed, Jan 16, 2013 at 1:55 PM, Michael Han <<a href="mailto:fragmentshaders@gmail.com" target="_blank">fragmentshaders@gmail.com</a>> wrote:<br>
> Cool thanks. Please take a look at attached patch, a brief description :<br>
> - Introduce a new bit field in Attr to store the index into the spelling<br>
> list of each attribute in Attr.td.<br>
> - Introduce a new method to AttributeList to generate the index based on<br>
> attribute syntax, name, and scope.<br>
> Implementation of this method is generated through table-gen.<br>
> - Teach table-gen to print an attribute based on this index.<br>
><br>
> Michael<br>
><br>
><br>
> On Mon, Jan 14, 2013 at 6:55 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>><br>
> wrote:<br>
>><br>
>> On Mon, Jan 14, 2013 at 11:40 AM, Michael Han <<a href="mailto:fragmentshaders@gmail.com" target="_blank">fragmentshaders@gmail.com</a>><br>
>> wrote:<br>
>>><br>
>>> Hi Richard,<br>
>>><br>
>>> I think that is a great idea but I am not sure what value to pass to Attr<br>
>>> when it's constructed in SemaDeclAttr.cpp. I was hoping to reuse the syntax<br>
>>> enumerator value as the spelling list index but it does not work for all<br>
>>> cases (e.g. alignment attribute in GNU syntax has two spellings.).<br>
>>><br>
>>> Alternatively, at the time we construct an Attr, there is enough<br>
>>> information (both the syntax and the actual spelling) we can use to print<br>
>>> the attribute in full fidelity. We can pass the spelling string to the Attr,<br>
>>> besides the syntax used as this patch did, so the printer will know which<br>
>>> syntax and spelling to select. It would be more elegant to encode both<br>
>>> information as a single index into the spelling list but I haven't figured<br>
>>> out how to do that. Any suggestions on this?<br>
>><br>
>><br>
>> You would need to teach TableGen to emit a function which maps the<br>
>> attribute syntax, scope, and name to a spelling index (and to use the<br>
>> spelling index when printing the attribute). You can then use that to add a<br>
>> method to AttributeList to get the spelling index for an attribute.<br>
>><br>
>>><br>
>>> Cheers<br>
>>> Michael<br>
>>><br>
>>><br>
>>> On Fri, Jan 11, 2013 at 5:14 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>><br>
>>> wrote:<br>
>>>><br>
>>>> Hi,<br>
>>>><br>
>>>> On Fri, Jan 11, 2013 at 3:53 PM, Michael Han <<a href="mailto:fragmentshaders@gmail.com" target="_blank">fragmentshaders@gmail.com</a>><br>
>>>> wrote:<br>
>>>>><br>
>>>>> Hi,<br>
>>>>><br>
>>>>> Attached patch is to fix PR14922. Currently when print an attribute the<br>
>>>>> GNU syntax will always be used, even if the attribute has no GNU syntax.The<br>
>>>>> fix is to pass the syntax flag when constructing the Attr node and take that<br>
>>>>> into consideration when printing the attribute. The name of actual attribute<br>
>>>>> gets printed is read from table gen definition file so there is still some<br>
>>>>> limitations, for example, when an attribute has multiple spellings, the<br>
>>>>> first spelling is used; and the namespace of the attribute (in case it's a<br>
>>>>> C++11 attribute) is not printed. I test the patch locally in my project<br>
>>>>> which has access to Clang AST but I am not sure how to write a stand alone<br>
>>>>> test to test the attribute pretty print. After this patch gets in I'll send<br>
>>>>> another patch which updates the SemaDeclAttr to pass the actual syntax flag<br>
>>>>> from AttributeList to Attr.<br>
>>>><br>
>>>><br>
>>>> I don't think this is the best approach: it still always uses the first<br>
>>>> spelling, so it still won't produce the right string for<br>
>>>> __attribute__((aligned(...))) versus __declspec(alignment(...)) versus<br>
>>>> [[gnu::aligned(...)]] versus alignas(...).<br>
>>>><br>
>>>> Since we already have a list of possible spellings for an attribute in<br>
>>>> the attribute definition (which incorporates the syntax used), how about<br>
>>>> just storing an index into that list on the Attr?<br>
>>><br>
>>><br>
>><br>
><br>
</div></div></blockquote></div><br>