[cfe-commits] [PATCH] Automatically generate Attr

[Douglas Gregor]

Hi Sean,

On Aug 14, 2010, at 2:50 AM, Sean Hunt wrote:
> This is a freakin' huge patch, the primary purpose of which is to make it so that subclasses of Attr are generated automagically by TableGen.
> 
> A few other changes slipped through from my main working branch and didn't seem like undoing (such as renaming Sema::MergeAttributes to Sema::MergeDeclAttributeS) since they will get done anyways in the future.
> 
> The most noticeable other change is in the way that attributes are stored; they are now stored in SmallVectors rather than as linked lists. I can't recall exactly why this is at the moment, but Doug Gregor and I had a long talk where we decided it was optimal, so assuming we were both sane at the time, it is the correct thing to do.*

I really wish I remembered this discussion :)

SmallVectors do make it easier to copy attributes around and keep the order right, and they could improve iteration performance (although I doubt it will matter).

> A nice extra interface was added to Decl as well as generally - the specific_attr_iterator. This is an iterator to iterate over a specific type of attr, much as specific_decl_iterator already does elsewhere.

Yep, good stuff.

> Some other notes about newly-generated attribute classes:
> 
> - The constructor arguments are a SourceLocation and a Context&,
>   followed by the attributes arguments in the order that they were
>   defined in Attr.td
> 
> - Every argument in Attr.td has an appropriate accessor, and there are
>   sometimes a few extra ones.
> 
> Also, it just occurs to me I've forgotten to add CMake build commands for the three new .inc files; it's very late so I'll hold off on those for the final version.

Okay, we'll want those in there when you commit, otherwise we'll break some builders (and a couple developers, too).

> One last note about EnumArguments for attributes - I will add an additional flag to specify where the value comes from - the attribute spelling, a string literal, or an identifier. This will come later, when TableGen-enhanced code becomes responsible for the parsing and semantic analysis of attributes.

Okay.

diff --git a/include/clang/AST/Attr.h b/include/clang/AST/Attr.h
index 699fff9..bd06f0f 100644
--- a/include/clang/AST/Attr.h
+++ b/include/clang/AST/Attr.h
@@ -15,8 +15,10 @@
 #define LLVM_CLANG_AST_ATTR_H
 
 #include "llvm/Support/Casting.h"
+#include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "clang/Basic/AttrKinds.h"
+#include "clang/Basic/SourceLocation.h"
 #include <cassert>
 #include <cstring>
 #include <algorithm>
@@ -27,25 +29,33 @@ namespace clang {
   class IdentifierInfo;
   class ObjCInterfaceDecl;
   class Expr;
+  class FunctionDecl;
+  class TypeSourceInfo;
 }
 
 // Defined in ASTContext.h
 void *operator new(size_t Bytes, clang::ASTContext &C,
-                   size_t Alignment = 16) throw ();
+                   size_t Alignment = 8) throw ();

Why are you changing the alignment here? I don't disagree with the change, but it should be orthogonal to your attributes work.
 
@@ -59,38 +69,36 @@ protected:
     assert(0 && "Attrs cannot be released with regular 'delete'.");
   }
 
+public:
+  // Forward so that the regular new and delete do not hide global ones.
+  void* operator new(size_t Bytes, ASTContext &C,
+                     size_t Alignment = 16) throw() {
+    return ::operator new(Bytes, C, Alignment);
+  }
+  void operator delete(void *Ptr, ASTContext &C,
+                       size_t Alignment = 16) throw() {
+    return ::operator delete(Ptr, C, Alignment);
+  }

... especially when you stuck with 16-byte alignment here :)

diff --git a/include/clang/AST/DeclBase.h b/include/clang/AST/DeclBase.h
index 3ff38dc..a362765 100644
--- a/include/clang/AST/DeclBase.h
+++ b/include/clang/AST/DeclBase.h
@@ -308,24 +308,52 @@ public:
   }
 
   bool hasAttrs() const { return HasAttrs; }
-  void initAttrs(Attr *attrs);
-  void addAttr(Attr *attr);
-  const Attr *getAttrs() const {
-    if (!HasAttrs) return 0;  // common case, no attributes.
-    return getAttrsImpl();    // Uncommon case, out of line hash lookup.
+  void setAttrs(const AttrVec& Attrs);
+  AttrVec& getAttrs() {
+    return const_cast<AttrVec&>(const_cast<const Decl*>(this)->getAttrs());
   }
+  const AttrVec &getAttrs() const;
   void swapAttrs(Decl *D);
-  void invalidateAttrs();
+  void dropAttrs();
 
-  template<typename T> const T *getAttr() const {
-    for (const Attr *attr = getAttrs(); attr; attr = attr->getNext())
-      if (const T *V = dyn_cast<T>(attr))
-        return V;
-    return 0;
+  void addAttr(Attr *A) {
+    if (hasAttrs())
+      getAttrs().push_back(A);
+    else
+      setAttrs(AttrVec(1, A));
   }
 
+  typedef AttrVec::const_iterator attr_iterator;
+
+  // FIXME: Do not rely on iterators having comparable singular values.
+  //        Note that this should error out if they do not.
+  attr_iterator attr_begin() const {
+    return hasAttrs() ? getAttrs().begin() : 0;
+  }
+  attr_iterator attr_end() const {
+    return hasAttrs() ? getAttrs().end() : 0;
+  }
+
+  template <typename T>
+  specific_attr_iterator<T> specific_attr_begin() const {
+    return specific_attr_iterator<T>(attr_begin());
+  }
+  template <typename T>
+  specific_attr_iterator<T> specific_attr_end() const {
+    return specific_attr_iterator<T>(attr_end());
+  }


A *true* C++ geek would name these attr_begin/attr_end, and dare his colleagues to call it "ambiguous" :)

--- a/include/clang/Basic/Attr.td
+++ b/include/clang/Basic/Attr.td
@@ -224,7 +228,7 @@ def IBOutlet : Attr {
 
 def IBOutletCollection : Attr {
   let Spellings = ["iboutletcollection"];
-  let Args = [ObjCInterfaceArgument<"Class">];
+  let Args = [ObjCInterfaceArgument<"Interface">];
 }
 
Note that this attribute got a type argument in Clang r111275.

@@ -290,26 +302,18 @@ def Override : Attr {
   let Spellings = ["override"];
   let Subjects = [CXXVirtualMethod];
   let Namespaces = ["", "std"];
-  let DoNotEmit = 0;
 }
 
 def Overloadable : Attr {
   let Spellings = ["overloadable"];
 }
 
-def OwnershipReturns : Attr {
-  let Spellings = ["ownership_returns"];
-  let Args = [StringArgument<"Module">, IntArgument<"SizeIdx">];
-}
-
-def OwnershipTakes : Attr {
-  let Spellings = ["ownership_takes"];
-  let Args = [StringArgument<"Module">, IntArgument<"PtrIdx">];
-}
-
-def OwnershipHolds : Attr {
-  let Spellings = ["ownership_holds"];
-  let Args = [StringArgument<"Module">, IntArgument<"PtrIdx">];
+def Ownership : Attr {
+  let Spellings = ["ownership_holds", "ownership_returns", "ownership_takes"];
+  let Args = [EnumArgument<"OwnKind", "OwnershipKind",
+                    ["ownership_holds", "ownership_returns", "ownership_takes"],
+                    ["Holds", "Returns", "Takes"]>,
+              StringArgument<"Module">, VariadicUnsignedArgument<"Args">];
 }

Very interesting! Any particular reason you wanted to combine these into a single attribute?
... ah, it actually does seem to clean up some of the uses.

--- a/include/clang/Frontend/PCHReader.h
+++ b/include/clang/Frontend/PCHReader.h
@@ -908,7 +908,7 @@ public:
   CXXTemporary *ReadCXXTemporary(const RecordData &Record, unsigned &Idx);
       
   /// \brief Reads attributes from the current stream position.
-  Attr *ReadAttributes(llvm::BitstreamCursor &DeclsCursor);
+  AttrVec ReadAttributes(llvm::BitstreamCursor &DeclsCursor);
 
   /// \brief Reads a statement.
   Stmt *ReadStmt(llvm::BitstreamCursor &Cursor);

Returning an AttrVec by-value could mean copying a bunch of data. Why not pass one in by reference?

--- a/include/clang/Frontend/PCHWriter.h
+++ b/include/clang/Frontend/PCHWriter.h
@@ -281,7 +281,7 @@ private:
   void WriteSelectors(Sema &SemaRef);
   void WriteReferencedSelectorsPool(Sema &SemaRef);
   void WriteIdentifierTable(Preprocessor &PP);
-  void WriteAttributeRecord(const Attr *Attr);
+  void WriteAttributeRecord(const AttrVec Attrs);
   void WriteDeclUpdateBlock();
 
   unsigned ParmVarDeclAbbrev;

Pass by reference-to-const, please!

--- a/lib/AST/ASTContext.cpp
+++ b/lib/AST/ASTContext.cpp
@@ -502,8 +502,7 @@ const llvm::fltSemantics &ASTContext::getFloatTypeSemantics(QualType T) const {
 CharUnits ASTContext::getDeclAlign(const Decl *D, bool RefAsPointee) {
   unsigned Align = Target.getCharWidth();
 
-  if (const AlignedAttr* AA = D->getAttr<AlignedAttr>())
-    Align = std::max(Align, AA->getMaxAlignment());
+  Align = std::max(Align, D->getMaxAlignment());
 
   if (const ValueDecl *VD = dyn_cast<ValueDecl>(D)) {
     QualType T = VD->getType();

This would have been a worthy cleanup on its own, but it's okay to commit with the rest of the patch.

I think this is a *great* cleanup of attribute class definition and (de-)serialization. Most of the comments above are minor, so please go ahead and commit when you've managed to address them.

	- Doug