test/Sema/ast-print.c, test/SemaCXX/ast-print.cpp, and test/SemaCXX/cxx11-ast-print.cpp look like good places to start.
On Wed, Jan 16, 2013 at 7:09 PM, Michael Han <[email protected]> wrote: > Agree, any suggestions on where / how to add the tests? I don't find any > existing regression tests that cover attributes print. > > Michael > > > On Wed, Jan 16, 2013 at 6:56 PM, Richard Smith <[email protected]> > wrote: >> >> On Wed, Jan 16, 2013 at 6:35 PM, Michael Han <[email protected]> >> wrote: >> > Attached patch should fix all issues pointed out in last review. Thanks! >> >> Great, thanks. This looks basically fine, but it will need tests >> before it can be checked in. (Also, there are a couple of stray blank >> lines added to lib/Sema/CMakeLists.txt.) >> >> > On Wed, Jan 16, 2013 at 4:01 PM, Richard Smith <[email protected]> >> > wrote: >> >> >> >> On Wed, Jan 16, 2013 at 3:27 PM, Michael Han >> >> <[email protected]> >> >> wrote: >> >> > Thanks! I have some inline comments below. >> >> > >> >> > >> >> > On Wed, Jan 16, 2013 at 2:27 PM, Richard Smith >> >> > <[email protected]> >> >> > wrote: >> >> >> >> >> >> Index: utils/TableGen/ClangAttrEmitter.cpp >> >> >> =================================================================== >> >> >> --- utils/TableGen/ClangAttrEmitter.cpp (revision 172543) >> >> >> +++ utils/TableGen/ClangAttrEmitter.cpp (working copy) >> >> >> @@ -735,6 +735,82 @@ >> >> >> [...] >> >> >> + OS << >> >> >> + " case(" << I << ") : {\n" >> >> >> + " OS << \"" + Prefix.str() + Spelling.str(); >> >> >> + >> >> >> + if (Args.size()) OS << "("; >> >> >> >> >> >> Please only generate parens if there are arguments. This has a >> >> >> semantic impact; for instance, [[noreturn()]] is ill-formed in >> >> >> C++11. >> >> > >> >> > >> >> > 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? >> >> >> >> Just a lack of caffeine on my part! :) >> > >> > >> > OK :) >> > >> >> >> >> >> >> >> @@ -1044,6 +1105,63 @@ >> >> >> [...] >> >> >> + // We only care about attributes that participate sema >> >> >> checking, >> >> >> so >> >> >> + // skip those attributes that are not able to make their way to >> >> >> Sema. >> >> >> >> >> >> ... participate *in* Sema checking >> >> >> >> >> >> + if (!R.getValueAsBit("SemaHandler")) >> >> >> + continue; >> >> >> >> >> >> Is this check necessary? >> >> > >> >> > >> >> > 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. >> >> >> >> OK, so we skip the !ASTNode items because there is no Attr subclass on >> >> which to store the spelling index, and we skip the !SemaHandler items >> >> because the parser itself ignores them, so they can't have a spelling. >> >> Makes sense, thanks. >> >> >> >> Presumably that means the check for ASTNode isn't essential? In fact, >> >> we might want to be able to ask an AttributeList for, say, AT_Mode for >> >> its spelling, so perhaps we should drop the ASTNode check? >> > >> > >> > Agree. The ASTNode check is removed. >> > >> >> >> >> >> >> >> + // Each distinct spelling yield an attribute kind. >> >> >> + if (R.getValueAsBit("DistinctSpellings")) { >> >> >> + for (unsigned I = 0; I < Spellings.size(); ++ I) { >> >> >> + OS << >> >> >> + " case AT_" << Spellings[I]->getValueAsString("Name") << >> >> >> ": >> >> >> {\n" >> >> >> + " Index = " << I << ";\n" >> >> >> + " break;\n" >> >> >> + "}\n"; >> >> >> >> >> >> I think all DistinctSpellings spellings should have spelling index >> >> >> 0. >> >> >> >> >> > >> >> > 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. >> >> >> >> They get different AT_ values but the same Attr subclass? OK, then >> >> this is fine. Thanks! >> >> >> >> >> + OS << " if (Name == \"" >> >> >> + << Spellings[I]->getValueAsString("Name") << "\" && " >> >> >> + << "AttrSyntax == \"" << >> >> >> Spellings[I]->getValueAsString("Variety") >> >> >> + << "\" && Scope == \"" << Namespace << "\")\n" >> >> >> + << " return " << I << ";\n"; >> >> >> >> >> >> Please convert the Variety into the syntax enumeration, rather than >> >> >> converting the syntax value into a string, and compare this first. >> >> >> It'd be great to use a StringSwitch here, too (or even something >> >> >> faster, since we know that one of the syntaxes must match). >> >> >> >> >> > >> >> > Thanks, I'll use the enumeration for now and think about produce >> >> > faster >> >> > code >> >> > here. >> > >> > >> > The updated patch uses StringSwitch for this. >> > >> >> >> >> > >> >> >> >> >> >> Index: lib/Sema/AttributeList.cpp >> >> >> =================================================================== >> >> >> --- lib/Sema/AttributeList.cpp (revision 172543) >> >> >> +++ lib/Sema/AttributeList.cpp (working copy) >> >> >> @@ -125,3 +125,39 @@ >> >> >> >> >> >> return ::getAttrKind(Buf); >> >> >> } >> >> >> + >> >> >> +unsigned AttributeList::getAttributeSpellListIndex() const { >> >> >> >> >> >> getAttributeSpellingListIndex? >> >> >> >> >> >> + switch (SyntaxUsed) { >> >> >> + default : { >> >> >> + llvm_unreachable("Unknown attribute syntax!"); >> >> >> + break; >> >> >> + } >> >> >> + case (AS_GNU) : { >> >> >> + AttrSyntax = "GNU"; >> >> >> + break; >> >> >> + } >> >> >> >> >> >> This should be formatted as: >> >> > >> >> > >> >> > Thanks for pointing this out! I'll kill the curly braces in tablegen >> >> > emitted >> >> > code too :) >> >> > >> >> >> >> >> >> switch (SyntaxUsed) { >> >> >> default: >> >> >> llvm_unreachable("Unknown attribute syntax!"); >> >> >> >> >> >> case AS_GNU: >> >> >> AttrSyntax = "GNU"; >> >> >> break; >> >> >> >> >> >> ... but, as noted above, don't do this conversion. >> >> >> >> >> > >> >> > Thanks! >> >> > Michael >> >> > >> >> >> >> >> >> On Wed, Jan 16, 2013 at 1:55 PM, Michael Han >> >> >> <[email protected]> >> >> >> wrote: >> >> >> > Cool thanks. Please take a look at attached patch, a brief >> >> >> > description : >> >> >> > - Introduce a new bit field in Attr to store the index into the >> >> >> > spelling >> >> >> > list of each attribute in Attr.td. >> >> >> > - Introduce a new method to AttributeList to generate the index >> >> >> > based >> >> >> > on >> >> >> > attribute syntax, name, and scope. >> >> >> > Implementation of this method is generated through table-gen. >> >> >> > - Teach table-gen to print an attribute based on this index. >> >> >> > >> >> >> > Michael >> >> >> > >> >> >> > >> >> >> > On Mon, Jan 14, 2013 at 6:55 PM, Richard Smith >> >> >> > <[email protected]> >> >> >> > wrote: >> >> >> >> >> >> >> >> On Mon, Jan 14, 2013 at 11:40 AM, Michael Han >> >> >> >> <[email protected]> >> >> >> >> wrote: >> >> >> >>> >> >> >> >>> Hi Richard, >> >> >> >>> >> >> >> >>> I think that is a great idea but I am not sure what value to >> >> >> >>> pass >> >> >> >>> to >> >> >> >>> Attr >> >> >> >>> when it's constructed in SemaDeclAttr.cpp. I was hoping to reuse >> >> >> >>> the >> >> >> >>> syntax >> >> >> >>> enumerator value as the spelling list index but it does not work >> >> >> >>> for >> >> >> >>> all >> >> >> >>> cases (e.g. alignment attribute in GNU syntax has two >> >> >> >>> spellings.). >> >> >> >>> >> >> >> >>> Alternatively, at the time we construct an Attr, there is enough >> >> >> >>> information (both the syntax and the actual spelling) we can use >> >> >> >>> to >> >> >> >>> print >> >> >> >>> the attribute in full fidelity. We can pass the spelling string >> >> >> >>> to >> >> >> >>> the >> >> >> >>> Attr, >> >> >> >>> besides the syntax used as this patch did, so the printer will >> >> >> >>> know >> >> >> >>> which >> >> >> >>> syntax and spelling to select. It would be more elegant to >> >> >> >>> encode >> >> >> >>> both >> >> >> >>> information as a single index into the spelling list but I >> >> >> >>> haven't >> >> >> >>> figured >> >> >> >>> out how to do that. Any suggestions on this? >> >> >> >> >> >> >> >> >> >> >> >> You would need to teach TableGen to emit a function which maps >> >> >> >> the >> >> >> >> attribute syntax, scope, and name to a spelling index (and to use >> >> >> >> the >> >> >> >> spelling index when printing the attribute). You can then use >> >> >> >> that >> >> >> >> to >> >> >> >> add a >> >> >> >> method to AttributeList to get the spelling index for an >> >> >> >> attribute. >> >> >> >> >> >> >> >>> >> >> >> >>> Cheers >> >> >> >>> Michael >> >> >> >>> >> >> >> >>> >> >> >> >>> On Fri, Jan 11, 2013 at 5:14 PM, Richard Smith >> >> >> >>> <[email protected]> >> >> >> >>> wrote: >> >> >> >>>> >> >> >> >>>> Hi, >> >> >> >>>> >> >> >> >>>> On Fri, Jan 11, 2013 at 3:53 PM, Michael Han >> >> >> >>>> <[email protected]> >> >> >> >>>> wrote: >> >> >> >>>>> >> >> >> >>>>> Hi, >> >> >> >>>>> >> >> >> >>>>> Attached patch is to fix PR14922. Currently when print an >> >> >> >>>>> attribute >> >> >> >>>>> the >> >> >> >>>>> GNU syntax will always be used, even if the attribute has no >> >> >> >>>>> GNU >> >> >> >>>>> syntax.The >> >> >> >>>>> fix is to pass the syntax flag when constructing the Attr node >> >> >> >>>>> and >> >> >> >>>>> take that >> >> >> >>>>> into consideration when printing the attribute. The name of >> >> >> >>>>> actual >> >> >> >>>>> attribute >> >> >> >>>>> gets printed is read from table gen definition file so there >> >> >> >>>>> is >> >> >> >>>>> still some >> >> >> >>>>> limitations, for example, when an attribute has multiple >> >> >> >>>>> spellings, >> >> >> >>>>> the >> >> >> >>>>> first spelling is used; and the namespace of the attribute (in >> >> >> >>>>> case >> >> >> >>>>> it's a >> >> >> >>>>> C++11 attribute) is not printed. I test the patch locally in >> >> >> >>>>> my >> >> >> >>>>> project >> >> >> >>>>> which has access to Clang AST but I am not sure how to write a >> >> >> >>>>> stand >> >> >> >>>>> alone >> >> >> >>>>> test to test the attribute pretty print. After this patch >> >> >> >>>>> gets >> >> >> >>>>> in >> >> >> >>>>> I'll send >> >> >> >>>>> another patch which updates the SemaDeclAttr to pass the >> >> >> >>>>> actual >> >> >> >>>>> syntax flag >> >> >> >>>>> from AttributeList to Attr. >> >> >> >>>> >> >> >> >>>> >> >> >> >>>> I don't think this is the best approach: it still always uses >> >> >> >>>> the >> >> >> >>>> first >> >> >> >>>> spelling, so it still won't produce the right string for >> >> >> >>>> __attribute__((aligned(...))) versus __declspec(alignment(...)) >> >> >> >>>> versus >> >> >> >>>> [[gnu::aligned(...)]] versus alignas(...). >> >> >> >>>> >> >> >> >>>> Since we already have a list of possible spellings for an >> >> >> >>>> attribute >> >> >> >>>> in >> >> >> >>>> the attribute definition (which incorporates the syntax used), >> >> >> >>>> how >> >> >> >>>> about >> >> >> >>>> just storing an index into that list on the Attr? >> >> >> >>> >> >> >> >>> >> >> >> >> >> >> >> > >> >> > >> >> > >> > >> > > > _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
