[cfe-commits] [Patch] PR14922 - Improve Attr::printPretty
Michael Han
fragmentshaders at gmail.com
Wed Jan 16 18:35:38 PST 2013
Attached patch should fix all issues pointed out in last review. Thanks!
Michael
On Wed, Jan 16, 2013 at 4:01 PM, Richard Smith <richard at metafoo.co.uk>wrote:
> On Wed, Jan 16, 2013 at 3:27 PM, Michael Han <fragmentshaders at gmail.com>
> wrote:
> > Thanks! I have some inline comments below.
> >
> >
> > On Wed, Jan 16, 2013 at 2:27 PM, Richard Smith <richard at metafoo.co.uk>
> > 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 <fragmentshaders at gmail.com
> >
> >> 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 <richard at metafoo.co.uk
> >
> >> > wrote:
> >> >>
> >> >> On Mon, Jan 14, 2013 at 11:40 AM, Michael Han
> >> >> <fragmentshaders at gmail.com>
> >> >> 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 <
> richard at metafoo.co.uk>
> >> >>> wrote:
> >> >>>>
> >> >>>> Hi,
> >> >>>>
> >> >>>> On Fri, Jan 11, 2013 at 3:53 PM, Michael Han
> >> >>>> <fragmentshaders at gmail.com>
> >> >>>> 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?
> >> >>>
> >> >>>
> >> >>
> >> >
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130116/0decbfe3/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: attr-print.patch
Type: application/octet-stream
Size: 13329 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130116/0decbfe3/attachment.obj>
More information about the cfe-commits
mailing list