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

[Richard Smith]

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.

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

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

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

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:

  switch (SyntaxUsed) {
  default:
    llvm_unreachable("Unknown attribute syntax!");

  case AS_GNU:
    AttrSyntax = "GNU";
    break;

... but, as noted above, don't do this conversion.

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?
>>>
>>>
>>
>