[cfe-commits] [Patch] PR14922 - Improve Attr::printPretty
Richard Smith
richard at metafoo.co.uk
Wed Jan 23 21:10:44 PST 2013
On Mon, Jan 21, 2013 at 8:12 PM, Michael Han <fragmentshaders at gmail.com> wrote:
> Attach updated patch with more tests added; also updated all attributes to
> include spelling index when their ast nodes are constructed.
> OK to commit?
+ OS <<
+ " case(" << I << ") : {\n"
Remove the parens here.
+ // end of the switch statement.
+ OS << "}\n";
+ // end of the print function.
Please start these comments with a capital letter.
Other than those tiny things, the patch looks good, but the test
changes seem to be missing from the diff?
> On Wed, Jan 16, 2013 at 7:12 PM, Richard Smith <richard at metafoo.co.uk>
> wrote:
>>
>> test/Sema/ast-print.c, test/SemaCXX/ast-print.cpp, and
>> test/SemaCXX/cxx11-ast-print.cpp look like good places to start.
>>
>> On Wed, Jan 16, 2013 at 7:09 PM, Michael Han <fragmentshaders at gmail.com>
>> wrote:
>> > 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 <richard at metafoo.co.uk>
>> > wrote:
>> >>
>> >> On Wed, Jan 16, 2013 at 6:35 PM, Michael Han
>> >> <fragmentshaders at gmail.com>
>> >> 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
>> >> > <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?
>> >> >> >> >>>
>> >> >> >> >>>
>> >> >> >> >>
>> >> >> >> >
>> >> >> >
>> >> >> >
>> >> >
>> >> >
>> >
>> >
>
>
More information about the cfe-commits
mailing list