[cfe-commits] [Patch] Fix parser diagnostic on class specifiers following c++11 attributes
Richard Smith
richard at metafoo.co.uk
Mon Nov 26 10:43:57 PST 2012
Just some tiny things, then:
+ // Skip C++11 attribute specifiers.
+ do {
+ if (Tok.is(tok::l_square) && NextToken().is(tok::l_square)) {
+ ConsumeBracket();
+ ConsumeBracket();
+ if (!SkipUntil(tok::r_square, false))
+ break;
This inner ConsumeBracket() / SkipUntil(...) pair is redundant. The
following SkipUntil call will do that for you.
+ if (!SkipUntil(tok::r_square, false))
You don't need the ', false' here. If we see a semicolon before the
expected ']', we should bail out.
+ break;
+ } else if ((Tok.is(tok::kw_alignas) || Tok.is(tok::kw__Alignas)) &&
+ NextToken().is(tok::l_paren)) {
+ ConsumeToken();
+ ConsumeParen();
+ if (!SkipUntil(tok::r_paren))
+ break;
+ } else {
+ break;
+ }
+ } while (true);
do { ... } while (true); is a bit of a funny way of writing this;
'while (true) { ... }' is more common in Clang sources.
@@ -1275,6 +1324,12 @@
} else
TUK = Sema::TUK_Reference;
+ // Forbid misplaced attributes. In cases of the reference, we pass attributes
+ // to caller to handle.
*"In the case of a reference, [...]"
On Tue, Nov 20, 2012 at 7:10 PM, Michael Han <fragmentshaders at gmail.com> wrote:
> You are right, we need to process two tokens at once. Good test case :)
> Attach updated patch, please take a look. Thanks!
>
> Cheers
> Michael
>
>
> On Tue, Nov 20, 2012 at 4:03 PM, Richard Smith <richard at metafoo.co.uk>
> wrote:
>>
>> On Mon, Nov 19, 2012 at 11:13 AM, Michael Han <fragmentshaders at gmail.com>
>> wrote:
>> > Thanks! Here is the updated patch that:
>> > - Enable ParseImplicitInt to pass attributes to caller.
>> > - Handle alignment specifiers as well.
>> > - Fix comments and variable naming Sean pointed out.
>> > - Update test case.
>>
>> + // Skip C++11 attribute specifiers.
>> + do {
>> + if (Tok.is(tok::l_square))
>> + ConsumeBracket();
>> + else
>> + ConsumeToken();
>> +
>> + if (Tok.is(tok::l_square)) {
>> + ConsumeBracket();
>> + SkipUntil(tok::r_square, false);
>> + SkipUntil(tok::r_square, false);
>> + }
>> +
>> + if (Tok.is(tok::l_paren)) {
>> + ConsumeParen();
>> + SkipUntil(tok::r_paren);
>> + }
>> + } while (Tok.is(tok::l_square) || Tok.is(tok::kw_alignas) ||
>> + Tok.is(tok::kw__Alignas));
>>
>> This doesn't look right. For instance, it looks like it will classify
>> 'struct S final [(int){0}];' as a definition. How about something more
>> like:
>>
>> while (true) {
>> if (Tok.is(tok::l_square) && NextToken().is(tok::l_square)) {
>> ConsumeBracket();
>> if (!SkipUntil(tok::r_square))
>> break;
>> } else if ((Tok.is(tok::kw_alignas) || Tok.is(tok::kw__Alignas)) &&
>> NextToken().is(tok::l_paren)) {
>> ConsumeToken();
>> ConsumeParen();
>> if (!SkipUntil(tok::r_paren))
>> break;
>> } else {
>> break;
>> }
>> }
>>
>> > On Sun, Nov 18, 2012 at 7:00 PM, Richard Smith <richard at metafoo.co.uk>
>> > wrote:
>> >>
>> >> Index: lib/Parse/ParseDecl.cpp
>> >> ===================================================================
>> >> --- lib/Parse/ParseDecl.cpp (revision 168292)
>> >> +++ lib/Parse/ParseDecl.cpp (working copy)
>> >> @@ -1923,12 +1923,13 @@
>> >> << TokenName << TagName;
>> >> }
>> >>
>> >> + ParsedAttributesWithRange attrs(AttrFactory);
>> >> // Parse this as a tag as if the missing tag were present.
>> >> if (TagKind == tok::kw_enum)
>> >> ParseEnumSpecifier(Loc, DS, TemplateInfo, AS, DSC_normal);
>> >> else
>> >> ParseClassSpecifier(TagKind, Loc, DS, TemplateInfo, AS,
>> >> - /*EnteringContext*/ false, DSC_normal);
>> >> + /*EnteringContext*/ false, DSC_normal,
>> >> attrs);
>> >> return true;
>> >> }
>> >>
>> >> It looks like the attributes are being discarded here. This is an
>> >> error path, but we have a fix-it so we still need to recover properly.
>> >> It'd be fine to add an extra parameter to ParseImplicitInt for this.
>> >>
>> >> @@ -1234,12 +1235,24 @@
>> >> // - If we have 'struct foo;', then this is either a forward
>> >> declaration
>> >> // or a friend declaration, which have to be treated differently.
>> >> // - Otherwise we have something like 'struct foo xyz', a
>> >> reference.
>> >> + // When start parsing from here, we also take misplaced C++11
>> >> attributes
>> >> + // specifiers into consideration by detecting the following cases:
>> >> + // - attributes follow class-name:
>> >> + // struct foo [[]] {};
>> >> + // - attributes appear before or after 'final':
>> >> + // struct foo [[]] final [[]] {};
>> >> + // This allow we produce better diagnostics.
>> >>
>> >> Something funny seems to have happened to the grammar in this comment.
>> >>
>> >> @@ -1261,6 +1274,30 @@
>> >> // Okay, this is a class definition.
>> >> TUK = Sema::TUK_Definition;
>> >> }
>> >> + } else if (isCXX0XFinalKeyword() && (NextToken().is(tok::l_square)))
>> >> {
>> >> + // We can't tell if this is a declaration or definition or
>> >> reference
>> >> + // until we skipped the 'final' and attributes sequences.
>> >>
>> >> This comment isn't quite right: this can't be a declaration if we have
>> >> the 'final' keyword here.
>> >>
>> >> + do {
>> >> + // Skip the attributes without parsing them
>> >> + if (NextToken().is(tok::l_square)) {
>> >> + ConsumeBracket();
>> >> + ConsumeBracket();
>> >> + SkipUntil(tok::r_square, false);
>> >> + SkipUntil(tok::r_square, false);
>> >>
>> >> The indentation here is off.
>> >>
>> >> + }
>> >> + } while (Tok.is(tok::l_square));
>> >>
>> >> It'd be good to handle alignment-specifiers (the other kind of C++11
>> >> attribute-specifier) here.
>> >>
>> >> On Sun, Nov 18, 2012 at 3:52 PM, Sean Silva <silvas at purdue.edu> wrote:
>> >> > + ParsedAttributesWithRange attrs(AttrFactory);
>> >> >
>> >> > Variable names should be uppercase, as per the coding standards:
>> >> >
>> >> >
>> >> > <http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly>
>> >> >
>> >> > -- Sean Silva
>> >> >
>> >> > On Sun, Nov 18, 2012 at 5:52 PM, Michael Han
>> >> > <fragmentshaders at gmail.com>
>> >> > wrote:
>> >> >> Hi Richard,
>> >> >>
>> >> >> Thanks for the suggestions!
>> >> >>
>> >> >> Attached updated patch. The parser now will parse C++11 attribute
>> >> >> specifiers, in the form of double squares that appear at all
>> >> >> possible
>> >> >> syntactic locations following class-name in a class specifier (also
>> >> >> before
>> >> >> or after 'final' keyword). In case of final keyword, I have not
>> >> >> found a
>> >> >> way
>> >> >> to classify the nature of the class specifier without using
>> >> >> tentative
>> >> >> parsing. Also updated test cases.
>> >> >>
>> >> >> I will look into fix-it in a separate patch.
>> >> >>
>> >> >> Cheers
>> >> >> Michael
>> >> >>
>> >> >>
>> >> >> On Thu, Nov 15, 2012 at 10:32 PM, Richard Smith
>> >> >> <richard at metafoo.co.uk>
>> >> >> wrote:
>> >> >>>
>> >> >>> On Thu, Nov 15, 2012 at 10:33 AM, Michael Han
>> >> >>> <fragmentshaders at gmail.com>
>> >> >>> wrote:
>> >> >>> > Hi,
>> >> >>> >
>> >> >>> > Consider this code
>> >> >>> > class [[foo]] c [[foo]];
>> >> >>> >
>> >> >>> > Clang generates diagnostic like this:
>> >> >>> > error: an attribute list cannot appear here
>> >> >>> > class [[foo]] c [[foo]];
>> >> >>> > ^~~~
>> >> >>> >
>> >> >>> > I think the first attribute is conforming, and it's the second
>> >> >>> > attribute
>> >> >>> > that should be diagnosed.
>> >> >>>
>> >> >>> Yes, I agree that it would be better to point the diagnostic at the
>> >> >>> second attribute-specifier-seq (and ideally to say that it's in the
>> >> >>> wrong place, and offer a fixit...).
>> >> >>>
>> >> >>> > Attached the patch that fixes this.
>> >> >>>
>> >> >>> I don't think your approach here is going to work. This is valid:
>> >> >>>
>> >> >>> class c {};
>> >> >>> class c [[ ]] x;
>> >> >>>
>> >> >>> I believe your patch would treat 'class c' as a declaration in the
>> >> >>> second line, which it is not.
>> >> >>>
>> >> >>> In order to correctly handle this, I suggest you parse attributes
>> >> >>> after the class name as well as before it, before trying to
>> >> >>> classify
>> >> >>> the reference. If you then find that it's a TUK_Reference, return
>> >> >>> the
>> >> >>> attributes you found after the class name to the caller, for them
>> >> >>> to
>> >> >>> handle appropriately. Otherwise, produce a diagnostic indicating
>> >> >>> that
>> >> >>> they were written in the wrong place (offering a fixit would be
>> >> >>> awesome...). For a class definition, you should probably look for
>> >> >>> attributes both before and after the optional 'final'.
>> >> >>>
>> >> >>> Thanks for looking into this!
>> >> >>
>> >> >>
>> >> >>
>> >> >> _______________________________________________
>> >> >> cfe-commits mailing list
>> >> >> cfe-commits at cs.uiuc.edu
>> >> >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>> >> >>
>> >
>> >
>
>
More information about the cfe-commits
mailing list