[AST] Introduce GenericSelectionExpr::Association

[riccibruno]

Summary

Introduce a new class GenericSelectionExpr::Association which bundle together
an association expression and its TypeSourceInfo.

An iterator GenericSelectionExpr::AssociationIterator is additionally added to make
it possible to iterate over ranges of Associations. This iterator is a kind of proxy
iterator which abstract how exactly the expressions and the TypeSourceInfos are stored.

[steveire]

Just in case you didn't know, you could have (and still can!) just update https://reviews.llvm.org/D57098 instead of creating a new PR.

[steveire]

lib/AST/ASTDumper.cpp

1465

Range for loops in this file generally use auto.

[aaron.ballman]

include/clang/AST/Expr.h

5099

Carrying over the conversation from D57098:

@aaron.ballman It seems like this should be pretty trivial to make into a random access iterator, or am I missing something?

@riccibruno Indeed, at the cost of some boilerplate. I can totally do this but from what I understood the motivation was to use this in ranges, and for this a forward iterator is good enough (although see the next comment)

You are correct, the primary motivation are range-based APIs. However, the iterator-based API should still behave in a reasonable manner and some algorithms do better with random access iterators than forward only. Even a bidirectional iterator would be preferred because I can at least use std::prev() on it.

5103

Carrying over the conversation from D57098:

@aaron.ballman Cute, but I suspect this may come back to bite us at some point. For instance, if someone thinks they're working with a real pointer, they're likely to expect pointer arithmetic to work when it won't (at least they'll get compile errors though).

@riccibruno Hmm, but pointer is just the return type of operator-> no ? Is it actually required to behave like a pointer ? The only requirement I can find is that It->x must be equivalent to (*It).x, which is true here.

I double-checked and you're right, this is not a requirement of the STL.

Also looking at the requirements for forward iterators I think that this iterator should actually be downgraded to an input iterator, because of the requirement that reference = T&.

My concern is that with the less functional iterators, common algorithms get more expensive. For instance, std::distance(), std::advance() become more expensive without random access, and things like std::prev() become impossible.

It seems like the view this needs to provide is a read-only access to the AssociationTy objects (because we need to construct them on the fly), but not the data contained within them. If the only thing you can get back from the iterator is a const pointer/reference/value type, then we could store a local "current association" object in the iterator and return a pointer/reference to that. WDYT?

[riccibruno]

include/clang/AST/Expr.h

5103

I am worried about lifetime issues with this approach. Returning a reference/pointer to an Association object stored in the iterator means that the reference/pointer will dangle as soon as the iterator goes out of scope. This is potentially surprising since the "container" (that is the GenericSelectionExpr) here will still be in scope. Returning a value is safer in this aspect.

I believe it should be possible to make the iterator a random access iterator, at least
if we are willing to ignore some requirements:

1.) For forward iterators and up, we must have reference = T& or const T&.
2.) For forward iterators and up, It1 == It2 if and only if *It1 and *It2 are bound to the same object.

[riccibruno]

Made GenericSelectionExpr::AssociationIterator a random access iterator.

[aaron.ballman]

Adding some additional reviewers to help tease out the iterator design questions.

include/clang/AST/Expr.h

5103

I am worried about lifetime issues with this approach. Returning a reference/pointer to an Association object stored in the iterator means that the reference/pointer will dangle as soon as the iterator goes out of scope. This is potentially surprising since the "container" (that is the GenericSelectionExpr) here will still be in scope. Returning a value is safer in this aspect.

That's valid.

I believe it should be possible to make the iterator a random access iterator, at least if we are willing to ignore some requirements:

1.) For forward iterators and up, we must have reference = T& or const T&.
2.) For forward iterators and up, It1 == It2 if and only if *It1 and *It2 are bound to the same object.

Yes, but then passing these iterators to STL algorithms will have UB because we claim to meet the requirements for random access iteration but don't actually meet the requirements. I am not certain what problems might arise from violating these requirements.

@dblaikie, @mclow.lists: do you have opinions on whether it's okay to not meet these requirements or suggestions on what we could do differently with the design?

[riccibruno]

Rebased on D57104. No other changes.

[dblaikie]

include/clang/AST/Expr.h

5084

Worth using any of the iterator helpers LLVM has? (iterator_facade or the like)

5103

My vote is usually "yeah, have a member inside the iterator you return a reference/pointer to" to meet the iterator requirements. Yes, it means if you keep a pointer/reference to it, that is invalidated when you increment the iterator. But that's well established in iterator semantics.

(might be shooting from the hip here/not fully understanding the nuances/tradeoffs in this case)

[riccibruno]

include/clang/AST/Expr.h

5084

I did try to use iteratore_facade but for some reason I was getting strange overload resolution failures with it.

In the end it did not save much and so I just rewrote the boiler-plate (especially given that if we end up going with an input iterator there is not going to be much boiler-plate).

5103

I believe there are 3 possibilities here:

Option 1 : Make the iterator an input iterator, and make operator* return a value.
Pros : Safe, compliant with the spec.
Cons : Morally we can do all of the operations of a random iterator.

Option 2 : Make the iterator a random access iterator, and make operator* return a value.
Pros : Safe wrt lifetime issues.
Cons : Do not complies with the two requirement of forward iterators mentioned above.

Option 3 : Make the iterator a random access iterator, and make operator* return a
reference to an object stored inside the iterator.
Pros : Compliant with the spec.
Cons : Nasty lifetime issues (see below)

I believe that option 3 is problematic. An example:

AssociationIterator It = ...;
const Association &Assoc = *It++;
// oops, Assoc is dangling since It++ returns a value,
// which goes out of scope at the end of the full expression.
// The same problem do not exists when operator* returns a
// value since the lifetime of the returned Association will be
// extended when bound to the reference.

Probably the safe thing to do is to go with option 1.

[aaron.ballman]

include/clang/AST/Expr.h

5103

Probably the safe thing to do is to go with option 1.

In the interests of solving the problem at hand, I think we should go with the safest option and can put a FIXME near the iterator_category that explains why it's an input iterator and that we'd like it to have stronger guarantees someday. It should meet our needs for the AST dumping refactoring; we can solve the harder problems if/when they arise in practice.

lib/AST/ASTDumper.cpp

1465

I believe const auto & should be safe here even with the call to dumpChild() because the lambda captures Assoc by copy (even if the iterator does not return a reference, the value will be lifetime extended because of the const reference). Same applies elsewhere.

[riccibruno]

Moved back to an input iterator. Addressed Aaron's remaining comments.

[riccibruno]

include/clang/AST/Expr.h

5103

Let's do this. I tried to summarize how we ended up with an input iterator.

lib/AST/ASTDumper.cpp

1465

Yes it is safe. Done.

[aaron.ballman]

include/clang/AST/Expr.h

5084

Does using the iterator_facade_base help now that we're back to an input iterator? It seems like that should be able to get rid of some of the boilerplate.

5100

Note -> FIXME

iterator. -> iterator, and it would be nice if we could strengthen the iterator category someday.

5101

do not -> does not

[riccibruno]

include/clang/AST/Expr.h

5084

I must be holding it wrong; for some reason the post-fix operator ++ is not getting found when I use iterator_facade_base. It also forces me to define operator== as a member instead of a non-member function. Do you mind terribly if I don't use it ? It only at best avoid me to write operator!= and operator++(int).

[riccibruno]

Update the comment in the iterator as per Aaron's comments.

[aaron.ballman]

include/clang/AST/Expr.h

5084

It also removes all of the typedefs and operator->(), so it does remove quite a bit of boilerplate. You shouldn't have to do anything special to get it to locate the postfix operator++ though (so long as you use public inheritance), which makes me wonder what's going on for your use. I would like to understand more about why this base class doesn't work here when it seems to work fine for the other uses in the code base.

[riccibruno]

include/clang/AST/Expr.h

5084

Right. Let me try again then. I will still have to provide operator-> though since the implementation from iterator_facade_base is not adequate as far as I can tell.

[aaron.ballman]

include/clang/AST/Expr.h

5084

Yeah, I think you're right about operator->(); because of the odd semantics of the Association object, the facade implementation won't work there because it would return a pointer to a temporary.

[riccibruno]

Use iterator_facade_base. It didn't work the first time because I forgot to bring in the name operator++ from iterator_facade_base into AssociationIteratorTy. Since name lookup occurs before overload resolution and stops as soon as something is found, the postfix operator++ from iterator_facade_base was hidden by the prefix operator++ from AssociationIteratorTy.

[riccibruno]

Removed duplicated comment in AssociationIteratorTy

[aaron.ballman]

LGTM aside from some possible minor nits. Thank you for working on this!

include/clang/AST/Expr.h

5120

Can this continue to return reference instead of the concrete type? (Same in the cast expression.)

5124

Can this return pointer instead?

5259

If you like being explicit, you could use llvm::make_range() here, but I don't insist. Same below.

[riccibruno]

include/clang/AST/Expr.h

5120

Not literally reference since the base is dependent, but typename BaseTy::reference should work. Is that what you mean ?

5259

I have no special preference on this, so llvm::make_range() it is.

[aaron.ballman]

include/clang/AST/Expr.h

5120

Yes, that's what I meant, sorry. :-)

[riccibruno]

include/clang/AST/Expr.h

5120

Right. I will land this today when I have some time. Thanks for the review !

[steveire]

This was subsequently reverted. Is there a status update? Rebasing and committing D56959 would unblock traverser work and would allow this to progress separately in parallel.

[riccibruno]

In D57106#1375142, @steveire wrote:

This was subsequently reverted. Is there a status update? Rebasing and committing D56959 would unblock traverser work and would allow this to progress separately in parallel.

Sorry about that! I am going to land it right now. I just didn't want to do it just before going to sleep.

include/clang/AST/Expr.h

unsigned numTrailingObjects(OverloadToken<Stmt *>) const {
     // are the associated expressions.
     return 1 + getNumAssocs();
   }
 
   unsigned numTrailingObjects(OverloadToken<TypeSourceInfo *>) const {
     return getNumAssocs();
   }
 
+  template <bool Const> class AssociationIteratorTy;
+  /// Bundle together an association expression and its TypeSourceInfo.
+  /// The Const template parameter is for the const and non-const versions
+  /// of AssociationTy.
+  template <bool Const> class AssociationTy {
+    friend class GenericSelectionExpr;
+    template <bool OtherConst> friend class AssociationIteratorTy;
+    using ExprPtrTy =
+        typename std::conditional<Const, const Expr *, Expr *>::type;
+    using TSIPtrTy = typename std::conditional<Const, const TypeSourceInfo *,
+                                               TypeSourceInfo *>::type;
+    ExprPtrTy E;
+    TSIPtrTy TSI;
+    bool Selected;
+    AssociationTy(ExprPtrTy E, TSIPtrTy TSI, bool Selected)
+        : E(E), TSI(TSI), Selected(Selected) {}
+
+  public:
+    ExprPtrTy getAssociationExpr() const { return E; }
+    TSIPtrTy getTypeSourceInfo() const { return TSI; }
+    QualType getType() const { return TSI ? TSI->getType() : QualType(); }
+    bool isSelected() const { return Selected; }
+    AssociationTy *operator->() { return this; }
+    const AssociationTy *operator->() const { return this; }
+  }; // class AssociationTy
+
+  /// Random access iterator over const and non-const associations. It creates
+  /// (Const)Association objects on the fly to abstract from the underlying
+  /// storage of Stmt * and TypeSourceInfo * in GenericSelectionExpr.
+  template <bool Const> class AssociationIteratorTy {
+    friend class GenericSelectionExpr;
+    using StmtPtrPtrTy =
+        typename std::conditional<Const, const Stmt *const *, Stmt **>::type;
+    using TSIPtrPtrTy =
+        typename std::conditional<Const, const TypeSourceInfo *const *,
+                                  TypeSourceInfo **>::type;
+    StmtPtrPtrTy E = nullptr;
+    TSIPtrPtrTy TSI = nullptr; // Kept in sync with E.
+    unsigned Offset = 0, SelectedOffset = 0;
+    AssociationIteratorTy(StmtPtrPtrTy E, TSIPtrPtrTy TSI, unsigned Offset,
+                          unsigned SelectedOffset)
+        : E(E), TSI(TSI), Offset(Offset), SelectedOffset(SelectedOffset) {}
+
+  public:
+    using iterator_type = AssociationIteratorTy;
+    using iterator_category = std::random_access_iterator_tag;
+    using value_type = AssociationTy<Const>;
+    using difference_type = std::ptrdiff_t;
+    using reference = AssociationTy<Const>;
+    using pointer = AssociationTy<Const>;
+    AssociationIteratorTy() = default;
+    reference operator*() const {
+      return reference(cast<Expr>(*E), *TSI, Offset == SelectedOffset);
+    }
+    pointer operator->() const { return **this; }
+    iterator_type &operator++() { return *this += 1; }
+    iterator_type operator++(int) {
+      iterator_type Tmp = *this;
+      ++*this;
+      return Tmp;
+    }
+    iterator_type &operator--() { return *this -= 1; }
+    iterator_type operator--(int) {
+      iterator_type Tmp = *this;
+      --*this;
+      return Tmp;
+    }
+    iterator_type &operator+=(difference_type N) {
+      E += N;
+      TSI += N;
+      Offset += N;
+      return *this;
+    }
+    iterator_type &operator-=(difference_type N) { return *this += -N; }
+    friend iterator_type operator+(iterator_type It, difference_type N) {
+      return It += N;
+    }
+    friend iterator_type operator-(iterator_type It, difference_type N) {
+      return It -= N;
+    }
+    friend iterator_type operator+(difference_type N, iterator_type It) {
+      return It += N;
+    }
+    friend difference_type operator-(iterator_type It1, iterator_type It2) {
+      return It1.E - It2.E;
+    }
+    reference operator[](difference_type N) const { return *(*this + N); }
+    friend bool operator==(iterator_type It1, iterator_type It2) {
+      return It1.E == It2.E;
+    }
+    friend bool operator!=(iterator_type It1, iterator_type It2) {
+      return !(It1 == It2);
+    }
+    friend bool operator<(iterator_type It1, iterator_type It2) {
+      return std::less<StmtPtrPtrTy>()(It1.E, It2.E);
+    }
+    friend bool operator>(iterator_type It1, iterator_type It2) {
+      return std::less<StmtPtrPtrTy>()(It2.E, It1.E);
+    }
+    friend bool operator<=(iterator_type It1, iterator_type It2) {
+      return !(It1 > It2);
+    }
+    friend bool operator>=(iterator_type It1, iterator_type It2) {
+      return !(It1 < It2);
+    }
+  }; // class AssociationIterator
+
   /// Build a non-result-dependent generic selection expression.
   GenericSelectionExpr(const ASTContext &Context, SourceLocation GenericLoc,
                        Expr *ControllingExpr,
                        ArrayRef<TypeSourceInfo *> AssocTypes,
                        ArrayRef<Expr *> AssocExprs, SourceLocation DefaultLoc,
                        SourceLocation RParenLoc,
                        bool ContainsUnexpandedParameterPack,
                        unsigned ResultIndex);
Create(const ASTContext &Context, SourceLocation GenericLoc,
          Expr *ControllingExpr, ArrayRef<TypeSourceInfo *> AssocTypes,
          ArrayRef<Expr *> AssocExprs, SourceLocation DefaultLoc,
          SourceLocation RParenLoc, bool ContainsUnexpandedParameterPack);
 
   /// Create an empty generic selection expression for deserialization.
   static GenericSelectionExpr *CreateEmpty(const ASTContext &Context,
                                            unsigned NumAssocs);
 
+  using Association = AssociationTy<false>;
+  using ConstAssociation = AssociationTy<true>;
+  using AssociationIterator = AssociationIteratorTy<false>;
+  using ConstAssociationIterator = AssociationIteratorTy<true>;
+  using association_range = llvm::iterator_range<AssociationIterator>;
+  using const_association_range =
+      llvm::iterator_range<ConstAssociationIterator>;
+
   /// The number of association expressions.
   unsigned getNumAssocs() const { return NumAssocs; }
 
   /// The zero-based index of the result expression's generic association in
   /// the generic selection's association list.  Defined only if the
   /// generic selection is not result-dependent.
   unsigned getResultIndex() const {
     assert(!isResultDependent() &&
ArrayRef<Expr *> getAssocExprs() const {
     return {reinterpret_cast<Expr *const *>(getTrailingObjects<Stmt *>() +
                                             AssocExprStartIndex),
             NumAssocs};
   }
   ArrayRef<TypeSourceInfo *> getAssocTypeSourceInfos() const {
     return {getTrailingObjects<TypeSourceInfo *>(), NumAssocs};
   }
 
-  Expr *getAssocExpr(unsigned i) {
-    return cast<Expr>(getTrailingObjects<Stmt *>()[AssocExprStartIndex + i]);
-  }
-  const Expr *getAssocExpr(unsigned i) const {
-    return cast<Expr>(getTrailingObjects<Stmt *>()[AssocExprStartIndex + i]);
-  }
-
-  TypeSourceInfo *getAssocTypeSourceInfo(unsigned i) {
-    return getTrailingObjects<TypeSourceInfo *>()[i];
-  }
-  const TypeSourceInfo *getAssocTypeSourceInfo(unsigned i) const {
-    return getTrailingObjects<TypeSourceInfo *>()[i];
-  }
-
-  QualType getAssocType(unsigned i) const {
-    const TypeSourceInfo *TSI = getAssocTypeSourceInfo(i);
-    return TSI ? TSI->getType() : QualType();
+  /// Return the Ith association expression with its TypeSourceInfo,
+  /// bundled together in GenericSelectionExpr::(Const)Association.
+  Association getAssociation(unsigned I) {
+    assert(I < getNumAssocs() &&
+           "Out-of-range index in GenericSelectionExpr::getAssociation!");
+    return Association(
+        cast<Expr>(getTrailingObjects<Stmt *>()[AssocExprStartIndex + I]),
+        getTrailingObjects<TypeSourceInfo *>()[I],
+        !isResultDependent() && (getResultIndex() == I));
+  }
+  ConstAssociation getAssociation(unsigned I) const {
+    assert(I < getNumAssocs() &&
+           "Out-of-range index in GenericSelectionExpr::getAssociation!");
+    return ConstAssociation(
+        cast<Expr>(getTrailingObjects<Stmt *>()[AssocExprStartIndex + I]),
+        getTrailingObjects<TypeSourceInfo *>()[I],
+        !isResultDependent() && (getResultIndex() == I));
+  }
+
+  association_range associations() {
+    AssociationIterator Begin(getTrailingObjects<Stmt *>() +
+                                  AssocExprStartIndex,
+                              getTrailingObjects<TypeSourceInfo *>(),
+                              /*Offset=*/0, ResultIndex);
+    AssociationIterator End(Begin.E + NumAssocs, Begin.TSI + NumAssocs,
+                            /*Offset=*/NumAssocs, ResultIndex);
+    return {Begin, End};
+  }
+
+  const_association_range associations() const {
+    ConstAssociationIterator Begin(getTrailingObjects<Stmt *>() +
+                                       AssocExprStartIndex,
+                                   getTrailingObjects<TypeSourceInfo *>(),
+                                   /*Offset=*/0, ResultIndex);
+    ConstAssociationIterator End(Begin.E + NumAssocs, Begin.TSI + NumAssocs,
+                                 /*Offset=*/NumAssocs, ResultIndex);
+    return {Begin, End};
   }
 
   SourceLocation getGenericLoc() const {
     return GenericSelectionExprBits.GenericLoc;
   }
   SourceLocation getDefaultLoc() const { return DefaultLoc; }
   SourceLocation getRParenLoc() const { return RParenLoc; }
   SourceLocation getBeginLoc() const { return getGenericLoc(); }

include/clang/AST/RecursiveASTVisitor.h

bool RecursiveASTVisitor<Derived>::TraverseInitListExpr(
   return true;
 }
 
 // GenericSelectionExpr is a special case because the types and expressions
 // are interleaved.  We also need to watch out for null types (default
 // generic associations).
 DEF_TRAVERSE_STMT(GenericSelectionExpr, {
   TRY_TO(TraverseStmt(S->getControllingExpr()));
-  for (unsigned i = 0; i != S->getNumAssocs(); ++i) {
-    if (TypeSourceInfo *TS = S->getAssocTypeSourceInfo(i))
-      TRY_TO(TraverseTypeLoc(TS->getTypeLoc()));
-    TRY_TO_TRAVERSE_OR_ENQUEUE_STMT(S->getAssocExpr(i));
+  for (GenericSelectionExpr::Association Assoc : S->associations()) {
+    if (TypeSourceInfo *TSI = Assoc.getTypeSourceInfo())
+      TRY_TO(TraverseTypeLoc(TSI->getTypeLoc()));
+    TRY_TO_TRAVERSE_OR_ENQUEUE_STMT(Assoc.getAssociationExpr());
   }
   ShouldVisitChildren = false;
 })
 
 // PseudoObjectExpr is a special case because of the weirdness with
 // syntactic expressions and opaque values.
 DEF_TRAVERSE_STMT(PseudoObjectExpr, {
   TRY_TO_TRAVERSE_OR_ENQUEUE_STMT(S->getSyntacticForm());

include/clang/AST/StmtDataCollectors.td

 //--- Miscellaneous Stmts ------------------------------------------------//
 class CXXFoldExpr {
   code Code = [{
     addData(S->isRightFold()); addData(S->getOperator());
   }];
 }
 class GenericSelectionExpr {
   code Code = [{
-    for (unsigned i = 0; i < S->getNumAssocs(); ++i) {
-      addData(S->getAssocType(i));
+    for (GenericSelectionExpr::ConstAssociation Assoc : S->associations()) {
+      addData(Assoc.getType());
     }
   }];
 }
 class LambdaExpr {
   code Code = [{
     for (const LambdaCapture &C : S->captures()) {
       addData(C.isPackExpansion());
       addData(C.getCaptureKind());

lib/AST/ASTDumper.cpp

 }
 
 void ASTDumper::VisitGenericSelectionExpr(const GenericSelectionExpr *E) {
   if (E->isResultDependent())
     OS << " result_dependent";
   dumpStmt(E->getControllingExpr());
   dumpTypeAsChild(E->getControllingExpr()->getType()); // FIXME: remove
 
-  for (unsigned I = 0, N = E->getNumAssocs(); I != N; ++I) {
+  for (auto Assoc : E->associations()) {
     dumpChild([=] {
-      if (const TypeSourceInfo *TSI = E->getAssocTypeSourceInfo(I)) {
+      if (const TypeSourceInfo *TSI = Assoc.getTypeSourceInfo()) {
         OS << "case ";
         NodeDumper.dumpType(TSI->getType());
       } else {
         OS << "default";
       }
 
-      if (!E->isResultDependent() && E->getResultIndex() == I)
+      if (Assoc.isSelected())
         OS << " selected";
 
-      if (const TypeSourceInfo *TSI = E->getAssocTypeSourceInfo(I))
+      if (const TypeSourceInfo *TSI = Assoc.getTypeSourceInfo())
         dumpTypeAsChild(TSI->getType());
-      dumpStmt(E->getAssocExpr(I));
+      dumpStmt(Assoc.getAssociationExpr());
     });
   }
 }
 
 //===----------------------------------------------------------------------===//
 // C++ Expressions
 //===----------------------------------------------------------------------===//
 

lib/AST/StmtPrinter.cpp

if (Node->isArgumentType()) {
     OS << " ";
     PrintExpr(Node->getArgumentExpr());
   }
 }
 
 void StmtPrinter::VisitGenericSelectionExpr(GenericSelectionExpr *Node) {
   OS << "_Generic(";
   PrintExpr(Node->getControllingExpr());
-  for (unsigned i = 0; i != Node->getNumAssocs(); ++i) {
+  for (GenericSelectionExpr::Association Assoc : Node->associations()) {
     OS << ", ";
-    QualType T = Node->getAssocType(i);
+    QualType T = Assoc.getType();
     if (T.isNull())
       OS << "default";
     else
       T.print(OS, Policy);
     OS << ": ";
-    PrintExpr(Node->getAssocExpr(i));
+    PrintExpr(Assoc.getAssociationExpr());
   }
   OS << ")";
 }
 
 void StmtPrinter::VisitArraySubscriptExpr(ArraySubscriptExpr *Node) {
   PrintExpr(Node->getLHS());
   OS << "[";
   PrintExpr(Node->getRHS());

lib/AST/StmtProfile.cpp

 
 void StmtProfiler::VisitBlockExpr(const BlockExpr *S) {
   VisitExpr(S);
   VisitDecl(S->getBlockDecl());
 }
 
 void StmtProfiler::VisitGenericSelectionExpr(const GenericSelectionExpr *S) {
   VisitExpr(S);
-  for (unsigned i = 0; i != S->getNumAssocs(); ++i) {
-    QualType T = S->getAssocType(i);
+  for (GenericSelectionExpr::ConstAssociation Assoc : S->associations()) {
+    QualType T = Assoc.getType();
     if (T.isNull())
       ID.AddPointer(nullptr);
     else
       VisitType(T);
-    VisitExpr(S->getAssocExpr(i));
+    VisitExpr(Assoc.getAssociationExpr());
   }
 }
 
 void StmtProfiler::VisitPseudoObjectExpr(const PseudoObjectExpr *S) {
   VisitExpr(S);
   for (PseudoObjectExpr::const_semantics_iterator
          i = S->semantics_begin(), e = S->semantics_end(); i != e; ++i)
     // Normally, we would not profile the source expressions of OVEs.

lib/Sema/SemaExprObjC.cpp

if (ParenExpr *pe = dyn_cast<ParenExpr>(e)) {
     Expr *sub = stripARCUnbridgedCast(uo->getSubExpr());
     return new (Context)
         UnaryOperator(sub, UO_Extension, sub->getType(), sub->getValueKind(),
                       sub->getObjectKind(), uo->getOperatorLoc(), false);
   } else if (GenericSelectionExpr *gse = dyn_cast<GenericSelectionExpr>(e)) {
     assert(!gse->isResultDependent());
 
     unsigned n = gse->getNumAssocs();
-    SmallVector<Expr*, 4> subExprs(n);
-    SmallVector<TypeSourceInfo*, 4> subTypes(n);
-    for (unsigned i = 0; i != n; ++i) {
-      subTypes[i] = gse->getAssocTypeSourceInfo(i);
-      Expr *sub = gse->getAssocExpr(i);
-      if (i == gse->getResultIndex())
+    SmallVector<Expr *, 4> subExprs;
+    SmallVector<TypeSourceInfo *, 4> subTypes;
+    subExprs.reserve(n);
+    subTypes.reserve(n);
+    for (GenericSelectionExpr::Association assoc : gse->associations()) {
+      subTypes.push_back(assoc.getTypeSourceInfo());
+      Expr *sub = assoc.getAssociationExpr();
+      if (assoc.isSelected())
         sub = stripARCUnbridgedCast(sub);
-      subExprs[i] = sub;
+      subExprs.push_back(sub);
     }
 
     return GenericSelectionExpr::Create(
         Context, gse->getGenericLoc(), gse->getControllingExpr(), subTypes,
         subExprs, gse->getDefaultLoc(), gse->getRParenLoc(),
         gse->containsUnexpandedParameterPack(), gse->getResultIndex());
   } else {
     assert(isa<ImplicitCastExpr>(e) && "bad form of unbridged cast!");

lib/Sema/SemaPseudoObject.cpp

Expr *rebuild(Expr *e) {
                                              uop->canOverflow());
       }
 
       if (GenericSelectionExpr *gse = dyn_cast<GenericSelectionExpr>(e)) {
         assert(!gse->isResultDependent());
         unsigned resultIndex = gse->getResultIndex();
         unsigned numAssocs = gse->getNumAssocs();
 
-        SmallVector<Expr*, 8> assocs(numAssocs);
-        SmallVector<TypeSourceInfo*, 8> assocTypes(numAssocs);
-
-        for (unsigned i = 0; i != numAssocs; ++i) {
-          Expr *assoc = gse->getAssocExpr(i);
-          if (i == resultIndex) assoc = rebuild(assoc);
-          assocs[i] = assoc;
-          assocTypes[i] = gse->getAssocTypeSourceInfo(i);
+        SmallVector<Expr *, 8> assocExprs;
+        SmallVector<TypeSourceInfo *, 8> assocTypes;
+        assocExprs.reserve(numAssocs);
+        assocTypes.reserve(numAssocs);
+
+        for (GenericSelectionExpr::Association assoc : gse->associations()) {
+          Expr *assocExpr = assoc.getAssociationExpr();
+          if (assoc.isSelected())
+            assocExpr = rebuild(assocExpr);
+          assocExprs.push_back(assocExpr);
+          assocTypes.push_back(assoc.getTypeSourceInfo());
         }
 
         return GenericSelectionExpr::Create(
             S.Context, gse->getGenericLoc(), gse->getControllingExpr(),
-            assocTypes, assocs, gse->getDefaultLoc(), gse->getRParenLoc(),
+            assocTypes, assocExprs, gse->getDefaultLoc(), gse->getRParenLoc(),
             gse->containsUnexpandedParameterPack(), resultIndex);
       }
 
       if (ChooseExpr *ce = dyn_cast<ChooseExpr>(e)) {
         assert(!ce->isConditionDependent());
 
         Expr *LHS = ce->getLHS(), *RHS = ce->getRHS();
         Expr *&rebuiltExpr = ce->isConditionTrue() ? LHS : RHS;

lib/Sema/TreeTransform.h

 TreeTransform<Derived>::TransformGenericSelectionExpr(GenericSelectionExpr *E) {
   ExprResult ControllingExpr =
     getDerived().TransformExpr(E->getControllingExpr());
   if (ControllingExpr.isInvalid())
     return ExprError();
 
   SmallVector<Expr *, 4> AssocExprs;
   SmallVector<TypeSourceInfo *, 4> AssocTypes;
-  for (unsigned i = 0; i != E->getNumAssocs(); ++i) {
-    TypeSourceInfo *TS = E->getAssocTypeSourceInfo(i);
-    if (TS) {
-      TypeSourceInfo *AssocType = getDerived().TransformType(TS);
+  for (GenericSelectionExpr::Association Assoc : E->associations()) {
+    TypeSourceInfo *TSI = Assoc.getTypeSourceInfo();
+    if (TSI) {
+      TypeSourceInfo *AssocType = getDerived().TransformType(TSI);
       if (!AssocType)
         return ExprError();
       AssocTypes.push_back(AssocType);
     } else {
       AssocTypes.push_back(nullptr);
     }
 
-    ExprResult AssocExpr = getDerived().TransformExpr(E->getAssocExpr(i));
+    ExprResult AssocExpr =
+        getDerived().TransformExpr(Assoc.getAssociationExpr());
     if (AssocExpr.isInvalid())
       return ExprError();
     AssocExprs.push_back(AssocExpr.get());
   }
 
   return getDerived().RebuildGenericSelectionExpr(E->getGenericLoc(),
                                                   E->getDefaultLoc(),
                                                   E->getRParenLoc(),