[cfe-commits] [Patch] PR14922 - Improve Attr::printPretty

[Michael Han]

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?


> @@ -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.


>
> +    // 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.

+        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.


> 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/d6beb6ca/attachment.html>