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
