[cfe-commits] [Patch] Fix parser diagnostic on class specifiers following c++11 attributes

[Richard Smith]

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