Attach updated patch with more tests added; also updated all attributes to include spelling index when their ast nodes are constructed.<br>OK to commit?<br><br>Michael<br><br><div class="gmail_quote">On Wed, Jan 16, 2013 at 7:12 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">test/Sema/ast-print.c, test/SemaCXX/ast-print.cpp, and<br>
test/SemaCXX/cxx11-ast-print.cpp look like good places to start.<br>
<div class="HOEnZb"><div class="h5"><br>
On Wed, Jan 16, 2013 at 7:09 PM, Michael Han <<a href="mailto:fragmentshaders@gmail.com">fragmentshaders@gmail.com</a>> wrote:<br>
> Agree, any suggestions on where / how to add the tests? I don't find any<br>
> existing regression tests that cover attributes print.<br>
><br>
> Michael<br>
><br>
><br>
> On Wed, Jan 16, 2013 at 6:56 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>><br>
> wrote:<br>
>><br>
>> On Wed, Jan 16, 2013 at 6:35 PM, Michael Han <<a href="mailto:fragmentshaders@gmail.com">fragmentshaders@gmail.com</a>><br>
>> wrote:<br>
>> > Attached patch should fix all issues pointed out in last review. Thanks!<br>
>><br>
>> Great, thanks. This looks basically fine, but it will need tests<br>
>> before it can be checked in. (Also, there are a couple of stray blank<br>
>> lines added to lib/Sema/CMakeLists.txt.)<br>
>><br>
>> > On Wed, Jan 16, 2013 at 4:01 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>><br>
>> > wrote:<br>
>> >><br>
>> >> On Wed, Jan 16, 2013 at 3:27 PM, Michael Han<br>
>> >> <<a href="mailto:fragmentshaders@gmail.com">fragmentshaders@gmail.com</a>><br>
>> >> wrote:<br>
>> >> > Thanks! I have some inline comments below.<br>
>> >> ><br>
>> >> ><br>
>> >> > On Wed, Jan 16, 2013 at 2:27 PM, Richard Smith<br>
>> >> > <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>><br>
>> >> > wrote:<br>
>> >> >><br>
>> >> >> 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<br>
>> >> >> C++11.<br>
>> >> ><br>
>> >> ><br>
>> >> > This code will not emit the parens if there is no arguments as<br>
>> >> > Args.size()<br>
>> >> > will be zero thus OS << "(" will not be executed. Is there an issue<br>
>> >> > here?<br>
>> >><br>
>> >> Just a lack of caffeine on my part! :)<br>
>> ><br>
>> ><br>
>> > OK :)<br>
>> ><br>
>> >><br>
>> >><br>
>> >> >> @@ -1044,6 +1105,63 @@<br>
>> >> >> [...]<br>
>> >> >> + // We only care about attributes that participate sema<br>
>> >> >> checking,<br>
>> >> >> so<br>
>> >> >> + // skip those attributes that are not able to make their way to<br>
>> >> >> Sema.<br>
>> >> >><br>
>> >> >> ... participate *in* Sema checking<br>
>> >> >><br>
>> >> >> + if (!R.getValueAsBit("SemaHandler"))<br>
>> >> >> + continue;<br>
>> >> >><br>
>> >> >> Is this check necessary?<br>
>> >> ><br>
>> >> ><br>
>> >> > I think it is necessary. Attributes with sema handler set to 0 never<br>
>> >> > made to<br>
>> >> > Sema, and as a result no Attr will be constructed for these<br>
>> >> > attributes.<br>
>> >><br>
>> >> OK, so we skip the !ASTNode items because there is no Attr subclass on<br>
>> >> which to store the spelling index, and we skip the !SemaHandler items<br>
>> >> because the parser itself ignores them, so they can't have a spelling.<br>
>> >> Makes sense, thanks.<br>
>> >><br>
>> >> Presumably that means the check for ASTNode isn't essential? In fact,<br>
>> >> we might want to be able to ask an AttributeList for, say, AT_Mode for<br>
>> >> its spelling, so perhaps we should drop the ASTNode check?<br>
>> ><br>
>> ><br>
>> > Agree. The ASTNode check is removed.<br>
>> ><br>
>> >><br>
>> >><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") <<<br>
>> >> >> ":<br>
>> >> >> {\n"<br>
>> >> >> + " Index = " << I << ";\n"<br>
>> >> >> + " break;\n"<br>
>> >> >> + "}\n";<br>
>> >> >><br>
>> >> >> I think all DistinctSpellings spellings should have spelling index<br>
>> >> >> 0.<br>
>> >> >><br>
>> >> ><br>
>> >> > There are many parsed attribute kinds for an attribute with<br>
>> >> > DistinctSpellings set to 1, but it is still a single parsed attribute<br>
>> >> > (and I<br>
>> >> > think in fact we only have one attribute with DistinctSpellings set<br>
>> >> > to 1<br>
>> >> > in<br>
>> >> > clang : the OwnershipAttr) and share a single spelling list that<br>
>> >> > contain<br>
>> >> > multiple spellings, with each spelling maps to a source form, so we<br>
>> >> > need<br>
>> >> > different indexes here.<br>
>> >><br>
>> >> They get different AT_ values but the same Attr subclass? OK, then<br>
>> >> this is fine. Thanks!<br>
>> >><br>
>> >> >> + OS << " if (Name == \""<br>
>> >> >> + << Spellings[I]->getValueAsString("Name") << "\" && "<br>
>> >> >> + << "AttrSyntax == \"" <<<br>
>> >> >> 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>
>> >> ><br>
>> >> > Thanks, I'll use the enumeration for now and think about produce<br>
>> >> > faster<br>
>> >> > code<br>
>> >> > here.<br>
>> ><br>
>> ><br>
>> > The updated patch uses StringSwitch for this.<br>
>> ><br>
>> >><br>
>> >> ><br>
>> >> >><br>
>> >> >> 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>
>> >> ><br>
>> >> ><br>
>> >> > Thanks for pointing this out! I'll kill the curly braces in tablegen<br>
>> >> > emitted<br>
>> >> > code too :)<br>
>> >> ><br>
>> >> >><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>
>> >> >><br>
>> >> ><br>
>> >> > Thanks!<br>
>> >> > Michael<br>
>> >> ><br>
>> >> >><br>
>> >> >> On Wed, Jan 16, 2013 at 1:55 PM, Michael Han<br>
>> >> >> <<a href="mailto:fragmentshaders@gmail.com">fragmentshaders@gmail.com</a>><br>
>> >> >> wrote:<br>
>> >> >> > Cool thanks. Please take a look at attached patch, a brief<br>
>> >> >> > description :<br>
>> >> >> > - Introduce a new bit field in Attr to store the index into the<br>
>> >> >> > spelling<br>
>> >> >> > list of each attribute in Attr.td.<br>
>> >> >> > - Introduce a new method to AttributeList to generate the index<br>
>> >> >> > based<br>
>> >> >> > 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<br>
>> >> >> > <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>><br>
>> >> >> > wrote:<br>
>> >> >> >><br>
>> >> >> >> On Mon, Jan 14, 2013 at 11:40 AM, Michael Han<br>
>> >> >> >> <<a href="mailto:fragmentshaders@gmail.com">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<br>
>> >> >> >>> pass<br>
>> >> >> >>> to<br>
>> >> >> >>> Attr<br>
>> >> >> >>> when it's constructed in SemaDeclAttr.cpp. I was hoping to reuse<br>
>> >> >> >>> the<br>
>> >> >> >>> syntax<br>
>> >> >> >>> enumerator value as the spelling list index but it does not work<br>
>> >> >> >>> for<br>
>> >> >> >>> all<br>
>> >> >> >>> cases (e.g. alignment attribute in GNU syntax has two<br>
>> >> >> >>> 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<br>
>> >> >> >>> to<br>
>> >> >> >>> print<br>
>> >> >> >>> the attribute in full fidelity. We can pass the spelling string<br>
>> >> >> >>> to<br>
>> >> >> >>> the<br>
>> >> >> >>> Attr,<br>
>> >> >> >>> besides the syntax used as this patch did, so the printer will<br>
>> >> >> >>> know<br>
>> >> >> >>> which<br>
>> >> >> >>> syntax and spelling to select. It would be more elegant to<br>
>> >> >> >>> encode<br>
>> >> >> >>> both<br>
>> >> >> >>> information as a single index into the spelling list but I<br>
>> >> >> >>> haven't<br>
>> >> >> >>> 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<br>
>> >> >> >> the<br>
>> >> >> >> attribute syntax, scope, and name to a spelling index (and to use<br>
>> >> >> >> the<br>
>> >> >> >> spelling index when printing the attribute). You can then use<br>
>> >> >> >> that<br>
>> >> >> >> to<br>
>> >> >> >> add a<br>
>> >> >> >> method to AttributeList to get the spelling index for an<br>
>> >> >> >> attribute.<br>
>> >> >> >><br>
>> >> >> >>><br>
>> >> >> >>> Cheers<br>
>> >> >> >>> Michael<br>
>> >> >> >>><br>
>> >> >> >>><br>
>> >> >> >>> On Fri, Jan 11, 2013 at 5:14 PM, Richard Smith<br>
>> >> >> >>> <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>><br>
>> >> >> >>> wrote:<br>
>> >> >> >>>><br>
>> >> >> >>>> Hi,<br>
>> >> >> >>>><br>
>> >> >> >>>> On Fri, Jan 11, 2013 at 3:53 PM, Michael Han<br>
>> >> >> >>>> <<a href="mailto:fragmentshaders@gmail.com">fragmentshaders@gmail.com</a>><br>
>> >> >> >>>> wrote:<br>
>> >> >> >>>>><br>
>> >> >> >>>>> Hi,<br>
>> >> >> >>>>><br>
>> >> >> >>>>> Attached patch is to fix PR14922. Currently when print an<br>
>> >> >> >>>>> attribute<br>
>> >> >> >>>>> the<br>
>> >> >> >>>>> GNU syntax will always be used, even if the attribute has no<br>
>> >> >> >>>>> GNU<br>
>> >> >> >>>>> syntax.The<br>
>> >> >> >>>>> fix is to pass the syntax flag when constructing the Attr node<br>
>> >> >> >>>>> and<br>
>> >> >> >>>>> take that<br>
>> >> >> >>>>> into consideration when printing the attribute. The name of<br>
>> >> >> >>>>> actual<br>
>> >> >> >>>>> attribute<br>
>> >> >> >>>>> gets printed is read from table gen definition file so there<br>
>> >> >> >>>>> is<br>
>> >> >> >>>>> still some<br>
>> >> >> >>>>> limitations, for example, when an attribute has multiple<br>
>> >> >> >>>>> spellings,<br>
>> >> >> >>>>> the<br>
>> >> >> >>>>> first spelling is used; and the namespace of the attribute (in<br>
>> >> >> >>>>> case<br>
>> >> >> >>>>> it's a<br>
>> >> >> >>>>> C++11 attribute) is not printed. I test the patch locally in<br>
>> >> >> >>>>> my<br>
>> >> >> >>>>> project<br>
>> >> >> >>>>> which has access to Clang AST but I am not sure how to write a<br>
>> >> >> >>>>> stand<br>
>> >> >> >>>>> alone<br>
>> >> >> >>>>> test to test the attribute pretty print. After this patch<br>
>> >> >> >>>>> gets<br>
>> >> >> >>>>> in<br>
>> >> >> >>>>> I'll send<br>
>> >> >> >>>>> another patch which updates the SemaDeclAttr to pass the<br>
>> >> >> >>>>> actual<br>
>> >> >> >>>>> syntax flag<br>
>> >> >> >>>>> from AttributeList to Attr.<br>
>> >> >> >>>><br>
>> >> >> >>>><br>
>> >> >> >>>> I don't think this is the best approach: it still always uses<br>
>> >> >> >>>> the<br>
>> >> >> >>>> first<br>
>> >> >> >>>> spelling, so it still won't produce the right string for<br>
>> >> >> >>>> __attribute__((aligned(...))) versus __declspec(alignment(...))<br>
>> >> >> >>>> versus<br>
>> >> >> >>>> [[gnu::aligned(...)]] versus alignas(...).<br>
>> >> >> >>>><br>
>> >> >> >>>> Since we already have a list of possible spellings for an<br>
>> >> >> >>>> attribute<br>
>> >> >> >>>> in<br>
>> >> >> >>>> the attribute definition (which incorporates the syntax used),<br>
>> >> >> >>>> how<br>
>> >> >> >>>> about<br>
>> >> >> >>>> just storing an index into that list on the Attr?<br>
>> >> >> >>><br>
>> >> >> >>><br>
>> >> >> >><br>
>> >> >> ><br>
>> >> ><br>
>> >> ><br>
>> ><br>
>> ><br>
><br>
><br>
</div></div></blockquote></div><br>