Append new attributes to the end of an AttributeList.

Recommit of r335084 after revert in r335516.

... instead of prepending it at the beginning (the original behavior
since implemented in r122535 2010-12-23). This builds up an
AttributeList in the the order in which the attributes appear in the
source.

The reverse order caused nodes for attributes in the AST (e.g. LoopHint)
to be in the reverse order, and therefore printed in the wrong order in
-ast-dump. Some TODO comments mention this. The order was explicitly
reversed for enable_if attribute overload resolution and name mangling,
which is not necessary anymore with this patch.

The change unfortunately has some secondary effect, especially on
diagnostic output. In the simplest cases, the CHECK lines or expected
diagnostic were changed to the the new output. If the kind of
error/warning changed, the attributes' order was changed instead.

This unfortunately causes some 'previous occurrence here' hints to be
textually after the main marker. This typically happens when attributes
are merged, but are incompatible to each other. Interchanging the role
of the the main and note SourceLocation will also cause the case where
two different declaration's attributes (in contrast to multiple
attributes of the same declaration) are merged to be reverse. There is
no easy fix because sometimes previous attributes are merged into a new
declaration's attribute list, sometimes new attributes are added to a
previous declaration's attribute list. Since 'previous occurrence here'
pointing to locations after the main marker is not rare, I left the
markers as-is; it is only relevant when the attributes are declared in
the same declaration anyway.

Differential Revision: https://reviews.llvm.org/D48100

llvm-svn: 338800

clang/include/clang/Sema/ParsedAttr.h

@@ -728,10 +728,6 @@ class ParsedAttributesView {
   ParsedAttr &operator[](SizeType pos) { return *AttrList[pos]; }
   const ParsedAttr &operator[](SizeType pos) const { return *AttrList[pos]; }
 
-  void addAtStart(ParsedAttr *newAttr) {
-    assert(newAttr);
-    AttrList.insert(AttrList.begin(), newAttr);
-  }
   void addAtEnd(ParsedAttr *newAttr) {
     assert(newAttr);
     AttrList.push_back(newAttr);
@@ -785,6 +781,23 @@ class ParsedAttributesView {
   iterator end() { return iterator(AttrList.end()); }
   const_iterator end() const { return const_iterator(AttrList.end()); }
 
+  ParsedAttr &front() {
+    assert(!empty());
+    return *AttrList.front();
+  }
+  const ParsedAttr &front() const {
+    assert(!empty());
+    return *AttrList.front();
+  }
+  ParsedAttr &back() {
+    assert(!empty());
+    return *AttrList.back();
+  }
+  const ParsedAttr &back() const {
+    assert(!empty());
+    return *AttrList.back();
+  }
+
   bool hasAttribute(ParsedAttr::Kind K) const {
     return llvm::any_of(
         AttrList, [K](const ParsedAttr *AL) { return AL->getKind() == K; });
@@ -826,7 +839,7 @@ class ParsedAttributes : public ParsedAttributesView {
                      SourceLocation ellipsisLoc = SourceLocation()) {
     ParsedAttr *attr = pool.create(attrName, attrRange, scopeName, scopeLoc,
                                    args, numArgs, syntax, ellipsisLoc);
-    addAtStart(attr);
+    addAtEnd(attr);
     return attr;
   }
 
@@ -842,7 +855,7 @@ class ParsedAttributes : public ParsedAttributesView {
     ParsedAttr *attr = pool.create(
         attrName, attrRange, scopeName, scopeLoc, Param, introduced, deprecated,
         obsoleted, unavailable, MessageExpr, syntax, strict, ReplacementExpr);
-    addAtStart(attr);
+    addAtEnd(attr);
     return attr;
   }
 
@@ -853,7 +866,7 @@ class ParsedAttributes : public ParsedAttributesView {
                      IdentifierLoc *Param3, ParsedAttr::Syntax syntax) {
     ParsedAttr *attr = pool.create(attrName, attrRange, scopeName, scopeLoc,
                                    Param1, Param2, Param3, syntax);
-    addAtStart(attr);
+    addAtEnd(attr);
     return attr;
   }
 
@@ -867,7 +880,7 @@ class ParsedAttributes : public ParsedAttributesView {
     ParsedAttr *attr = pool.createTypeTagForDatatype(
         attrName, attrRange, scopeName, scopeLoc, argumentKind, matchingCType,
         layoutCompatible, mustBeNull, syntax);
-    addAtStart(attr);
+    addAtEnd(attr);
     return attr;
   }
 
@@ -878,7 +891,7 @@ class ParsedAttributes : public ParsedAttributesView {
                              ParsedAttr::Syntax syntaxUsed) {
     ParsedAttr *attr = pool.createTypeAttribute(attrName, attrRange, scopeName,
                                                 scopeLoc, typeArg, syntaxUsed);
-    addAtStart(attr);
+    addAtEnd(attr);
     return attr;
   }
 
@@ -891,7 +904,7 @@ class ParsedAttributes : public ParsedAttributesView {
     ParsedAttr *attr =
         pool.createPropertyAttribute(attrName, attrRange, scopeName, scopeLoc,
                                      getterId, setterId, syntaxUsed);
-    addAtStart(attr);
+    addAtEnd(attr);
     return attr;
   }
 

clang/lib/AST/ItaniumMangle.cpp

@@ -721,10 +721,8 @@ void CXXNameMangler::mangleFunctionEncodingBareType(const FunctionDecl *FD) {
   if (FD->hasAttr<EnableIfAttr>()) {
     FunctionTypeDepthState Saved = FunctionTypeDepth.push();
     Out << "Ua9enable_ifI";
-    // FIXME: specific_attr_iterator iterates in reverse order. Fix that and use
-    // it here.
-    for (AttrVec::const_reverse_iterator I = FD->getAttrs().rbegin(),
-                                         E = FD->getAttrs().rend();
+    for (AttrVec::const_iterator I = FD->getAttrs().begin(),
+                                 E = FD->getAttrs().end();
          I != E; ++I) {
       EnableIfAttr *EIA = dyn_cast<EnableIfAttr>(*I);
       if (!EIA)

clang/lib/Parse/ParseDeclCXX.cpp

@@ -3875,7 +3875,7 @@ bool Parser::ParseCXX11AttributeArgs(IdentifierInfo *AttrName,
 
   if (!Attrs.empty() &&
       IsBuiltInOrStandardCXX11Attribute(AttrName, ScopeName)) {
-    ParsedAttr &Attr = *Attrs.begin();
+    ParsedAttr &Attr = Attrs.back();
     // If the attribute is a standard or built-in attribute and we are
     // parsing an argument list, we need to determine whether this attribute
     // was allowed to have an argument list (such as [[deprecated]]), and how

clang/lib/Parse/ParseObjc.cpp

@@ -384,12 +384,12 @@ static void addContextSensitiveTypeNullability(Parser &P,
 
   if (D.getNumTypeObjects() > 0) {
     // Add the attribute to the declarator chunk nearest the declarator.
-    D.getTypeObject(0).getAttrs().addAtStart(
+    D.getTypeObject(0).getAttrs().addAtEnd(
         getNullabilityAttr(D.getAttributePool()));
   } else if (!addedToDeclSpec) {
     // Otherwise, just put it on the declaration specifiers (if one
     // isn't there already).
-    D.getMutableDeclSpec().getAttributes().addAtStart(
+    D.getMutableDeclSpec().getAttributes().addAtEnd(
         getNullabilityAttr(D.getMutableDeclSpec().getAttributes().getPool()));
     addedToDeclSpec = true;
   }
@@ -1198,7 +1198,7 @@ static void takeDeclAttributes(ParsedAttributesView &attrs,
   for (auto &AL : llvm::reverse(from)) {
     if (!AL.isUsedAsTypeAttr()) {
       from.remove(&AL);
-      attrs.addAtStart(&AL);
+      attrs.addAtEnd(&AL);
     }
   }
 }

clang/lib/Sema/SemaAttr.cpp

@@ -661,7 +661,7 @@ void Sema::AddPragmaAttributes(Scope *S, Decl *D) {
     Entry.IsUsed = true;
     PragmaAttributeCurrentTargetDecl = D;
     ParsedAttributesView Attrs;
-    Attrs.addAtStart(Attribute);
+    Attrs.addAtEnd(Attribute);
     ProcessDeclAttributeList(S, D, Attrs);
     PragmaAttributeCurrentTargetDecl = nullptr;
   }

clang/lib/Sema/SemaOverload.cpp

@@ -6215,24 +6215,6 @@ Sema::SelectBestMethod(Selector Sel, MultiExprArg Args, bool IsInstance,
   return nullptr;
 }
 
-// specific_attr_iterator iterates over enable_if attributes in reverse, and
-// enable_if is order-sensitive. As a result, we need to reverse things
-// sometimes. Size of 4 elements is arbitrary.
-static SmallVector<EnableIfAttr *, 4>
-getOrderedEnableIfAttrs(const FunctionDecl *Function) {
-  SmallVector<EnableIfAttr *, 4> Result;
-  if (!Function->hasAttrs())
-    return Result;
-
-  const auto &FuncAttrs = Function->getAttrs();
-  for (Attr *Attr : FuncAttrs)
-    if (auto *EnableIf = dyn_cast<EnableIfAttr>(Attr))
-      Result.push_back(EnableIf);
-
-  std::reverse(Result.begin(), Result.end());
-  return Result;
-}
-
 static bool
 convertArgsForAvailabilityChecks(Sema &S, FunctionDecl *Function, Expr *ThisArg,
                                  ArrayRef<Expr *> Args, Sema::SFINAETrap &Trap,
@@ -6306,9 +6288,8 @@ convertArgsForAvailabilityChecks(Sema &S, FunctionDecl *Function, Expr *ThisArg,
 
 EnableIfAttr *Sema::CheckEnableIf(FunctionDecl *Function, ArrayRef<Expr *> Args,
                                   bool MissingImplicitThis) {
-  SmallVector<EnableIfAttr *, 4> EnableIfAttrs =
-      getOrderedEnableIfAttrs(Function);
-  if (EnableIfAttrs.empty())
+  auto EnableIfAttrs = Function->specific_attrs<EnableIfAttr>();
+  if (EnableIfAttrs.begin() == EnableIfAttrs.end())
     return nullptr;
 
   SFINAETrap Trap(*this);
@@ -6318,7 +6299,7 @@ EnableIfAttr *Sema::CheckEnableIf(FunctionDecl *Function, ArrayRef<Expr *> Args,
   if (!convertArgsForAvailabilityChecks(
           *this, Function, /*ThisArg=*/nullptr, Args, Trap,
           /*MissingImplicitThis=*/true, DiscardedThis, ConvertedArgs))
-    return EnableIfAttrs[0];
+    return *EnableIfAttrs.begin();
 
   for (auto *EIA : EnableIfAttrs) {
     APValue Result;
@@ -8967,24 +8948,21 @@ static Comparison compareEnableIfAttrs(const Sema &S, const FunctionDecl *Cand1,
     return Cand1Attr ? Comparison::Better : Comparison::Worse;
   }
 
-  // FIXME: The next several lines are just
-  // specific_attr_iterator<EnableIfAttr> but going in declaration order,
-  // instead of reverse order which is how they're stored in the AST.
-  auto Cand1Attrs = getOrderedEnableIfAttrs(Cand1);
-  auto Cand2Attrs = getOrderedEnableIfAttrs(Cand2);
-
-  // It's impossible for Cand1 to be better than (or equal to) Cand2 if Cand1
-  // has fewer enable_if attributes than Cand2.
-  if (Cand1Attrs.size() < Cand2Attrs.size())
-    return Comparison::Worse;
+  auto Cand1Attrs = Cand1->specific_attrs<EnableIfAttr>();
+  auto Cand2Attrs = Cand2->specific_attrs<EnableIfAttr>();
 
   auto Cand1I = Cand1Attrs.begin();
   llvm::FoldingSetNodeID Cand1ID, Cand2ID;
-  for (auto &Cand2A : Cand2Attrs) {
+  for (EnableIfAttr *Cand2A : Cand2Attrs) {
     Cand1ID.clear();
     Cand2ID.clear();
 
-    auto &Cand1A = *Cand1I++;
+    // It's impossible for Cand1 to be better than (or equal to) Cand2 if Cand1
+    // has fewer enable_if attributes than Cand2.
+    auto Cand1A = Cand1I++;
+    if (Cand1A == Cand1Attrs.end())
+      return Comparison::Worse;
+
     Cand1A->getCond()->Profile(Cand1ID, S.getASTContext(), true);
     Cand2A->getCond()->Profile(Cand2ID, S.getASTContext(), true);
     if (Cand1ID != Cand2ID)

clang/lib/Sema/SemaType.cpp

@@ -246,7 +246,7 @@ namespace {
 
       getMutableDeclSpec().getAttributes().clearListOnly();
       for (ParsedAttr *AL : savedAttrs)
-        getMutableDeclSpec().getAttributes().addAtStart(AL);
+        getMutableDeclSpec().getAttributes().addAtEnd(AL);
     }
   };
 } // end anonymous namespace
@@ -255,7 +255,7 @@ static void moveAttrFromListToList(ParsedAttr &attr,
                                    ParsedAttributesView &fromList,
                                    ParsedAttributesView &toList) {
   fromList.remove(&attr);
-  toList.addAtStart(&attr);
+  toList.addAtEnd(&attr);
 }
 
 /// The location of a type attribute.
@@ -4128,7 +4128,7 @@ static TypeSourceInfo *GetFullTypeForDeclarator(TypeProcessingState &state,
               SourceRange(pointerLoc), nullptr, SourceLocation(), nullptr, 0,
               syntax);
 
-      attrs.addAtStart(nullabilityAttr);
+      attrs.addAtEnd(nullabilityAttr);
 
       if (inferNullabilityCS) {
         state.getDeclarator().getMutableDeclSpec().getObjCQualifiers()
@@ -5093,7 +5093,7 @@ static void transferARCOwnershipToDeclaratorChunk(TypeProcessingState &state,
       &S.Context.Idents.get("objc_ownership"), SourceLocation(),
       /*scope*/ nullptr, SourceLocation(),
       /*args*/ &Args, 1, ParsedAttr::AS_GNU);
-  chunk.getAttrs().addAtStart(attr);
+  chunk.getAttrs().addAtEnd(attr);
   // TODO: mark whether we did this inference?
 }
 

clang/lib/Serialization/ASTReaderDecl.cpp

@@ -2832,36 +2832,25 @@ static bool hasSameOverloadableAttrs(const FunctionDecl *A,
   // Note that pass_object_size attributes are represented in the function's
   // ExtParameterInfo, so we don't need to check them here.
 
-  SmallVector<const EnableIfAttr *, 4> AEnableIfs;
-  // Since this is an equality check, we can ignore that enable_if attrs show up
-  // in reverse order.
-  for (const auto *EIA : A->specific_attrs<EnableIfAttr>())
-    AEnableIfs.push_back(EIA);
-
-  SmallVector<const EnableIfAttr *, 4> BEnableIfs;
-  for (const auto *EIA : B->specific_attrs<EnableIfAttr>())
-    BEnableIfs.push_back(EIA);
-
-  // Two very common cases: either we have 0 enable_if attrs, or we have an
-  // unequal number of enable_if attrs.
-  if (AEnableIfs.empty() && BEnableIfs.empty())
-    return true;
-
-  if (AEnableIfs.size() != BEnableIfs.size())
-    return false;
-
+  // Return false if any of the enable_if expressions of A and B are different.
   llvm::FoldingSetNodeID Cand1ID, Cand2ID;
-  for (unsigned I = 0, E = AEnableIfs.size(); I != E; ++I) {
+  auto AEnableIfAttrs = A->specific_attrs<EnableIfAttr>();
+  auto BEnableIfAttrs = B->specific_attrs<EnableIfAttr>();
+  auto AEnableIf = AEnableIfAttrs.begin();
+  auto BEnableIf = BEnableIfAttrs.begin();
+  for (; AEnableIf != AEnableIfAttrs.end() && BEnableIf != BEnableIfAttrs.end();
+       ++BEnableIf, ++AEnableIf) {
     Cand1ID.clear();
     Cand2ID.clear();
 
-    AEnableIfs[I]->getCond()->Profile(Cand1ID, A->getASTContext(), true);
-    BEnableIfs[I]->getCond()->Profile(Cand2ID, B->getASTContext(), true);
+    AEnableIf->getCond()->Profile(Cand1ID, A->getASTContext(), true);
+    BEnableIf->getCond()->Profile(Cand2ID, B->getASTContext(), true);
     if (Cand1ID != Cand2ID)
       return false;
   }
 
-  return true;
+  // Return false if the number of enable_if attributes was different.
+  return AEnableIf == AEnableIfAttrs.end() && BEnableIf == BEnableIfAttrs.end();
 }
 
 /// Determine whether the two declarations refer to the same entity.

clang/test/Index/annotate-comments-availability-attrs.cpp

@@ -13,7 +13,7 @@
 void attr_availability_1() __attribute__((availability(macosx,obsoleted=10.0,introduced=8.0,deprecated=9.0, message="use availability_test in <foo.h>")))
                            __attribute__((availability(ios,unavailable, message="not for iOS")));
 
-// CHECK: FullCommentAsXML=[<Function file="{{[^"]+}}annotate-comments-availability-attrs.cpp" line="[[@LINE-3]]" column="6"><Name>attr_availability_1</Name><USR>c:@F@attr_availability_1#</USR><Declaration>void attr_availability_1()</Declaration><Abstract><Para> Aaa.</Para></Abstract><Availability distribution="iOS"><DeprecationSummary>not for iOS</DeprecationSummary><Unavailable/></Availability><Availability distribution="macOS"><IntroducedInVersion>8.0</IntroducedInVersion><DeprecatedInVersion>9.0</DeprecatedInVersion><RemovedAfterVersion>10.0</RemovedAfterVersion><DeprecationSummary>use availability_test in &lt;foo.h&gt;</DeprecationSummary></Availability></Function>]
+// CHECK: FullCommentAsXML=[<Function file="{{[^"]+}}annotate-comments-availability-attrs.cpp" line="[[@LINE-3]]" column="6"><Name>attr_availability_1</Name><USR>c:@F@attr_availability_1#</USR><Declaration>void attr_availability_1()</Declaration><Abstract><Para> Aaa.</Para></Abstract><Availability distribution="macOS"><IntroducedInVersion>8.0</IntroducedInVersion><DeprecatedInVersion>9.0</DeprecatedInVersion><RemovedAfterVersion>10.0</RemovedAfterVersion><DeprecationSummary>use availability_test in &lt;foo.h&gt;</DeprecationSummary></Availability><Availability distribution="iOS"><DeprecationSummary>not for iOS</DeprecationSummary><Unavailable/></Availability></Function>]
 
 /// Aaa.
 void attr_availability_2() __attribute__((availability(macosx,obsoleted=10.0.1,introduced=8.0.1,deprecated=9.0.1)));

clang/test/Index/complete-with-annotations.cpp

@@ -14,7 +14,7 @@ void X::doSomething() {
 }
 
 // CHECK: CXXMethod:{ResultType void}{TypedText doSomething}{LeftParen (}{RightParen )} (34)
-// CHECK: FieldDecl:{ResultType int}{TypedText field} (35) ("three", "two", "one")
+// CHECK: FieldDecl:{ResultType int}{TypedText field} (35) ("one", "two", "three")
 // CHECK: CXXMethod:{ResultType void}{TypedText func2}{LeftParen (}{RightParen )} (34) ("some annotation")
 // CHECK: FieldDecl:{ResultType int}{TypedText member2} (35) ("another annotation", "some annotation")
 // CHECK: CXXMethod:{ResultType X &}{TypedText operator=}{LeftParen (}{Placeholder const X &}{RightParen )} (79)

clang/test/Misc/ast-print-pragmas.cpp

@@ -1,11 +1,8 @@
 // RUN: %clang_cc1 -ast-print %s -o - | FileCheck %s
 // RUN: %clang_cc1 -DMS_EXT -fsyntax-only -fms-extensions %s -triple x86_64-pc-win32 -ast-print | FileCheck %s --check-prefix=MS-EXT
 
-// FIXME: A bug in ParsedAttributes causes the order of the attributes to be
-// reversed. The checks are consequently in the reverse order below.
-
-// CHECK: #pragma clang loop interleave_count(8){{$}}
-// CHECK-NEXT: #pragma clang loop vectorize_width(4)
+// CHECK: #pragma clang loop vectorize_width(4)
+// CHECK-NEXT: #pragma clang loop interleave_count(8){{$}}
 
 void test(int *List, int Length) {
   int i = 0;
@@ -17,9 +14,9 @@ void test(int *List, int Length) {
     i++;
   }
 
-// CHECK: #pragma clang loop interleave(disable)
+// CHECK: #pragma clang loop distribute(disable)
 // CHECK-NEXT: #pragma clang loop vectorize(enable)
-// CHECK-NEXT: #pragma clang loop distribute(disable)
+// CHECK-NEXT: #pragma clang loop interleave(disable)
 
 #pragma clang loop distribute(disable)
 #pragma clang loop vectorize(enable)
@@ -30,9 +27,9 @@ void test(int *List, int Length) {
     i++;
   }
 
-// CHECK: #pragma clang loop interleave(enable)
+// CHECK: #pragma clang loop distribute(enable)
 // CHECK-NEXT: #pragma clang loop vectorize(disable)
-// CHECK-NEXT: #pragma clang loop distribute(enable)
+// CHECK-NEXT: #pragma clang loop interleave(enable)
 
 #pragma clang loop distribute(enable)
 #pragma clang loop vectorize(disable)
@@ -52,8 +49,8 @@ void test_nontype_template_param(int *List, int Length) {
   }
 }
 
-// CHECK: #pragma clang loop interleave_count(I)
 // CHECK: #pragma clang loop vectorize_width(V)
+// CHECK: #pragma clang loop interleave_count(I)
 
 void test_templates(int *List, int Length) {
   test_nontype_template_param<2, 4>(List, Length);