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

[Douglas Gregor]

On Nov 17, 2009, at 7:14 AM, Sean Hunt wrote:

> This patch is the newest version of my attributes patch, with  
> questions/comments addressed. Reviews would be appreciated so that I  
> can commit. Thanks.

Just a few comments below, then feel free to commit. Thanks!

	- Doug

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

"can't" -> "cannot"

Index: lib/AST/RecordLayoutBuilder.cpp
===================================================================
--- lib/AST/RecordLayoutBuilder.cpp	(revision 89176)
+++ lib/AST/RecordLayoutBuilder.cpp	(working copy)
@@ -447,6 +447,8 @@
    if (const PragmaPackAttr *PPA = D->getAttr<PragmaPackAttr>())
      MaxFieldAlignment = PPA->getAlignment();

+  // FIXME: Accomodate attributes that weaken alignment rather than
+  //        strengthen it.
    if (const AlignedAttr *AA = D->getAttr<AlignedAttr>())
      UpdateAlignment(AA->getAlignment());

@@ -497,6 +499,8 @@
    if (const PragmaPackAttr *PPA = D->getAttr<PragmaPackAttr>())
      MaxFieldAlignment = PPA->getAlignment();

+  // FIXME: Accomodate attributes that weaken alignment rather than
+  //        strengthen it.
    if (const AlignedAttr *AA = D->getAttr<AlignedAttr>())
      UpdateAlignment(AA->getAlignment());

"Accommodate"

@@ -539,8 +543,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>());
+    }
      // The maximum field alignment overrides the aligned attribute.
      if (MaxFieldAlignment)
        FieldAlign = std::min(FieldAlign, MaxFieldAlignment);

We have this loop over the AlignedAttrs of a Decl in several places.  
Please encapsulate this behavior into a member function  
AlignedAttr::getFullAlignment(), or something like that.

+CXX0XAttributeList Parser::ParseCXX0XAttributes(SourceLocation  
*EndLoc) {
+  assert(Tok.is(tok::l_square) && NextToken().is(tok::l_square)
+      && "Not a C++0x attribute list");
+
+  SourceLocation StartLoc = Tok.getLocation(), Loc;
+  AttributeList *CurrAttr = 0;
+
+  ConsumeBracket();
+  ConsumeBracket();
+
+  while (Tok.is(tok::identifier) || Tok.is(tok::comma)) {
+    // attribute not present
+    if (Tok.is(tok::comma)) {
+      ConsumeToken();
+      continue;
+    }
+
+    IdentifierInfo *ScopeName = 0;
+    SourceLocation ScopeLoc;
+    // scoped attribute
+    if (NextToken().is(tok::coloncolon)) {
+      ScopeName = Tok.getIdentifierInfo();

NextToken() is expensive, so we should use it sparingly. Why not just  
consume the identifier that we see, then look for tok::coloncolon?