[cfe-commits] [Patch] PR14922 - Improve Attr::printPretty
Richard Smith
richard at metafoo.co.uk
Wed Jan 16 16:01:27 PST 2013
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! :)
>> @@ -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?
>> + // 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.
>
>>
>> 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?
>> >>>
>> >>>
>> >>
>> >
>
>
More information about the cfe-commits
mailing list