[cfe-commits] [patch] C++0x Attributes

[Douglas Gregor]

On Oct 15, 2009, at 8:04 PM, Sean Hunt wrote:
> Attached is my first "ready" version of the C++0x attributes patch.  
> It's certainly not complete (and there's lots of FIXMEs), but the  
> code works, and adds some neat new features like [[final]] to clang.

Nifty.

@@ -2197,7 +2209,7 @@
def ext_return_has_void_expr : Extension<
   "void %select{function|method}1 %0 should not return void  
expression">;
def warn_noreturn_function_has_return_expr : Warning<
-  "function %0 declared 'noreturn' should not return">, DefaultError,
+  "function %0 declared 'noreturn' should not return">,

I'm still going to ask that this be separated; feel free to go ahead  
and commit this change by itself.

@@ -239,6 +240,11 @@
def err_anon_type_definition : Error<
   "declaration of anonymous %0 must be a definition">;

+def err_cxx0x_attribute_forbids_arguments : Error<
+  "C++0x attribute '%0' can't have an argument list">;

"cannot"

Index: include/clang/AST/Attr.h
===================================================================
--- include/clang/AST/Attr.h	(revision 84241)
+++ include/clang/AST/Attr.h	(working copy)
@@ -58,6 +58,7 @@
     Deprecated,
     Destructor,
     FastCall,
+    Final,
     Format,
     FormatArg,
     GNUInline,

So carries_dependency isn't getting into the AST; that's fine.

Index: lib/CodeGen/CodeGenModule.cpp
===================================================================
--- lib/CodeGen/CodeGenModule.cpp	(revision 84241)
+++ lib/CodeGen/CodeGenModule.cpp	(working copy)
@@ -347,8 +347,11 @@
   if (D->hasAttr<NoInlineAttr>())
     F->addFnAttr(llvm::Attribute::NoInline);

-  if (const AlignedAttr *AA = D->getAttr<AlignedAttr>())
+  if (const AlignedAttr *AA = D->getAttr<AlignedAttr>()) {
     F->setAlignment(AA->getAlignment()/8);
+    while ((AA = AA->getNext<AlignedAttr>()))
+      F->setAlignment(std::max(F->getAlignment(), AA->getAlignment()/ 
8));
+  }

Do we really want to hardcode that 8? :)

Index: lib/Sema/SemaDeclCXX.cpp
===================================================================
--- lib/Sema/SemaDeclCXX.cpp	(revision 84241)
+++ lib/Sema/SemaDeclCXX.cpp	(working copy)
@@ -478,6 +478,11 @@
     Class->setEmpty(false);
   if (CXXBaseDecl->isPolymorphic())
     Class->setPolymorphic(true);
+  // C++0x CWG Issue #817 indicates that [[final]] classes shouldn't  
be bases.
+  if (CXXBaseDecl->hasAttr<FinalAttr>()) {
+    Diag(BaseLoc, diag::err_final_base) << BaseType.getAsString();
+    return 0;
+  }


It'd be nice to have a note here that points to the declaration of the  
base (CXXBaseDecl->getLocation()).

+static void HandleDependencyAttr(Decl *d, const AttributeList &Attr,  
Sema &S) {
+  if (!isFunctionOrMethod(d) && !isa<ParmVarDecl>(d)) {
+    S.Diag(Attr.getLoc(), diag::err_attribute_wrong_decl_type)
+      << Attr.getName() << 8; /*function, method, or parameter*/
+    return;
+  }
+  // FIXME: Actually store the attribute on the declaration
+}
+

@@ -531,8 +535,11 @@

     if (FieldPacked)
       FieldAlign = 1;
-    if (const AlignedAttr *AA = D->getAttr<AlignedAttr>())
-      FieldAlign = std::max(FieldAlign, AA->getAlignment());
+    if (const AlignedAttr *AA = D->getAttr<AlignedAttr>()) {
+      do {
+        FieldAlign = std::max(FieldAlign, AA->getAlignment());
+      } while (AA->getNext<AlignedAttr>());
+    }

I see at least four loops of this kind... could you abstract out the  
"compute alignment for a decl based on an initial value and its  
attributes" code?

+CXX0XAttributeList Parser::ParseCXX0XAttributes(SourceLocation  
*EndLoc) {

In this function, the parsing of "aligned" is rather large and deeply  
nested; could you move it to a separate function?

Index: lib/Parse/ParseDeclCXX.cpp
===================================================================
--- lib/Parse/ParseDeclCXX.cpp	(revision 84241)
+++ lib/Parse/ParseDeclCXX.cpp	(working copy)
@@ -68,7 +68,7 @@
     attrTok = Tok;

     // FIXME: save these somewhere.
-    AttrList = ParseAttributes();
+    AttrList = ParseGNUAttributes();
   }

   if (Tok.is(tok::equal)) {
@@ -96,8 +96,12 @@
                                         PP.getSourceManager(),
                                         "parsing namespace");

-  while (Tok.isNot(tok::r_brace) && Tok.isNot(tok::eof))
-    ParseExternalDeclaration();
+  while (Tok.isNot(tok::r_brace) && Tok.isNot(tok::eof)) {
+    CXX0XAttributeList Attr;
+    if (getLang().CPlusPlus0x && isCXX0XAttributeSpecifier())
+      Attr = ParseCXX0XAttributes();
+    ParseExternalDeclaration(Attr);
+  }

I find it a little bit odd that we parse the attributes at this level,  
rather than parsing them inside ParseExternalDeclaration. Any  
particular reason for this decision?

+  tok::TokenKind AfterTok = Tok.getKind();
+  bool HasAttr = false;
+
+  // Check to see if there is an optional attribute-specifier (C++0x  
only).
+  // We have to know now so that we can look ahead and see if we  
actually want
+  // to parse it.
+  if (getLang().CPlusPlus0x)
+    HasAttr = isCXX0XAttributeSpecifier(&AfterTok);

This is in ParseClassSpecifier, and there's good news here: CWG  
discussed issue 962 today, and the attributes for class and enum  
specifiers will be moving to the same place where GNU puts them,  
between the class-key/"enum" keyword and the identifier. That will  
eliminate the need for lookahead here (yay!).

Speaking of lookahead... we seem to be calling  
isCXX0XAttributeSpecifier() in a bunch of places where "[[" will  
always end up starting at attribute, which means we're doing a bunch  
of unnecessary lookahead. As attributes become more popular, this  
could actually matter. Could we have a bool argument to  
isCXX0XAttributeSpecifier to tell it whether it should just look for  
the prefix "[[" vs. having to look for the full attribute-specifier?

@@ -391,8 +394,8 @@
/// [C++0x] empty-declaration:
///           ';'
///
-/// [C++0x/GNU] 'extern' 'template' declaration
-Parser::DeclGroupPtrTy Parser::ParseExternalDeclaration() {
+/// [C++0x/GNU] 'extern' 'temp;ate' declaration
+Parser::DeclGroupPtrTy Parser::ParseExternalDeclaration 
(CXX0XAttributeList Attr) {

Typo "temp;ate".

We seem to be missing test cases? Otherwise, this is looking very good.

	- Doug