[cfe-commits] C++0x Attributes Patch - INCOMPLETE

[Douglas Gregor]

On Sep 4, 2009, at 4:29 PM, Sean Hunt wrote:
> Known bugs:
> - Attributes are not supported after the declarator-id in a  
> declaration
> - Attributes anywhere in a declaration apply to the entire declaration
> (e.g. int * [[noreturn]] foo (); is a noreturn function, even though  
> it
> should error out because [[noreturn]] is being applied to a pointer).
> - [[align(type)]] is not supported
>
> Notes:
> - Attributes are allowed in front of function definitions, like they
> are on declarations. This is in contrast to the draft, but I believe  
> the
> committee will make this legal in the next draft

Okay. I don't mind leading the draft on this, since the committee is  
likely to fix the problem.

> - The [[final]] attribute has the behavior suggested by a core  
> language
> issue (no time to look it up, sorry), rather than the draft (it  
> prevents
> any class from deriving from it, rather than making all virtual
> functions [[final]]).

Yeah, this is the only reasonable definition of [[final]] anyway.

> - No non-standard attributes are currently supported with C++0x  
> syntax.


Why not? I guess I'd expected that the C++0x attribute syntax would be  
just another syntax with the same semantics as GNU __attribute__s.

A few comments:

+def err_final_function_overridden : Error<
+  "declaration of %0 overrides a function declared with the 'final'  
attribute">;

How about:

   "declaration of %0 overrides a 'final' function"

?

+def err_final_base : Error<
+  "'%0' cannot be used as a base class because it was declared with "
+  "the 'final' attribute">;

How about:

   "derivation from 'final' class %0"

?

  def warn_noreturn_function_has_return_expr : Warning<
-  "function %0 declared 'noreturn' should not return">, DefaultError,
+  "function %0 declared 'noreturn' should not return">,
    InGroup<DiagGroup<"invalid-noreturn">>;
  def warn_falloff_noreturn_function : Warning<
-  "function declared 'noreturn' should not return">, DefaultError,
+  "function declared 'noreturn' should not return">,
    InGroup<DiagGroup<"invalid-noreturn">>;
  def err_noreturn_block_has_return_expr : Error<
    "block declared 'noreturn' should not return">;

I'm okay with this change, since those are the C++0x rules, but please  
submit/commit it as a separate patch.

+def err_cxx0x_attribute_requires_arguments : Error<
+  "C++0x requires that the attribute '%0' has an argument list">;

Should that be:

   "C++0x requires that the attribute '%0' have an attribute list"

?

+def warn_align_type_not_supported : Warning<
+  "align attribute not supported for types. Try align(alignof(type))  
instead">;

Oooh, is there a code modification hint for this?

+/// This does not do a full check for an attribute-specifier, as [[]]  
is
+/// unique within the langague and internal parse errors should be  
saved

Typo "langague"

+bool Parser::isCXX0XAttributeSpecifier (tok::TokenKind *After) {
+  struct TentativeReverter {
+    TentativeParsingAction PA;
+
+    TentativeReverter (Parser& P)
+      : PA(P)
+    {}
+    ~TentativeReverter () {
+      PA.Revert();
+    }
+  } R(*this);
+
+  if (Tok.isNot(tok::l_square))
+    return false;
+  ConsumeBracket();
+  if (Tok.isNot(tok::l_square))
+    return false;
+  ConsumeBracket();
+
+  SkipUntil(tok::r_square, false);
+
+  if (Tok.isNot(tok::r_square))
+    return false;
+  ConsumeBracket();
+
+  if (After)
+    *After = Tok.getKind();
+
+  return true;
+}

This is way simpler than I expected... I guess that's because  
SkipUntil already deals with balanced token sequences. Could you add a  
comment to that effect?

+  if (getLang().CPlusPlus0x && isCXX0XAttributeSpecifier()) {
+    SourceLocation Loc = Tok.getLocation();
+    CXX0XAttributeList Attr = ParseCXX0XAttributes();
+    if (CXX0XAttributesAllowed)
+      DS.AddAttributes(Attr.AttrList);
+    else
+      Diag(Loc, diag::err_attributes_not_allowed);
+  }

With this diagnostic, it would be nice if we could highlight the  
source range covering all of the attributes (from [[ to ]]).

+  // Possible draft standard bug - attribute-specifier should be  
allowed here?
+  OwningStmtResult Block(ParseCompoundStatement(0));

You should probably put "FIXME: " in front of that comment (there's  
another, similar comment above), so we don't forget about this issue.

This patch looks to be in very good shape. Can't wait to see the final  
version!

	- Doug