[AST] Pack GenericSelectionExpr

[riccibruno]

Summary

Store the controlling expression, the association expressions and the corresponding
TypeSourceInfos as trailing objects.

Additionally use the bit-fields of Stmt to store one SourceLocation, saving one
additional pointer. This saves 3 pointers in total per GenericSelectionExpr.

[steveire]

Splitting the introduction of and porting to Create would significantly reduce the number of files touched by the 'real' change in this commit, and therefore reduce noise in the commit (following the idea of "do one thing per commit" to make the code reviewable in the future).

However, if you're opposed to that, it's not a hard requirement.

include/clang/AST/Expr.h

5048 ↗

(On Diff #183119)

Would it be correct to use ASSOC_EXPR_START here instead of the magic 1?

[steveire]

I haven't done a full review as it's not obvious what parts of the diff relate to which separate change. Perhaps Aaron will review and approve though for you anyway.

Thanks!

[riccibruno]

In D57104#1368055, @steveire wrote:

Splitting the introduction of and porting to Create would significantly reduce the number of files touched by the 'real' change in this commit, and therefore reduce noise in the commit (following the idea of "do one thing per commit" to make the code reviewable in the future).

However, if you're opposed to that, it's not a hard requirement.

To be honest I don't really see the point. This is just the usual pattern of having a Create function doing the allocation,
which then dispatch to the private constructor with a placement new for the initialization.

include/clang/AST/Expr.h

5048 ↗

(On Diff #183119)

Eh maybe ?

[aaron.ballman]

include/clang/AST/Expr.h

5043 ↗

(On Diff #183119)

Do we want to use ControllingIndex and AssocExprStartIndex and combine with the enum you introduced above?

5048 ↗

(On Diff #183119)

I think using this named constant adds confusion rather than clarifies things because it's not clear what the index has to do with the count. I think the comments suffice here rather than introducing a named constant with a sufficiently generic name.

lib/AST/Expr.cpp

3814 ↗

(On Diff #183119)

-1U -> ResultDependentIndex

lib/Serialization/ASTReaderStmt.cpp

1034 ↗

(On Diff #183119)

You should add a comment above the loop that explains the + 1 is for the controlling expression and that's why it's not needed for the type source information.

lib/Serialization/ASTWriterStmt.cpp

979 ↗

(On Diff #183119)

Similar comment here as above.

[steveire]

In D57104#1368072, @riccibruno wrote:

In D57104#1368055, @steveire wrote:

Splitting the introduction of and porting to Create would significantly reduce the number of files touched by the 'real' change in this commit, and therefore reduce noise in the commit (following the idea of "do one thing per commit" to make the code reviewable in the future).

However, if you're opposed to that, it's not a hard requirement.

To be honest I don't really see the point.

Yes, the Create refactor a simple change. The point is to remove the noise of the simple change from the complex change so that the complex change becomes visible.

It would help reviewers like myself who are less familiar with what refactoring for tail allocation involves. I couldn't read your commit and know how to do it for another class because this commit is full of noise.

Anyway, if making the code reviewable (also in the future!) like that is not important enough, I won't block this one! :)

include/clang/AST/Expr.h

5095 ↗

(On Diff #183119)

Why remove the LLVM_READONLY from this class instead of from everywhere in the file it is not needed? (in a separate commit, obviously).

[riccibruno]

In D57104#1368292, @steveire wrote:

In D57104#1368072, @riccibruno wrote:

In D57104#1368055, @steveire wrote:

Splitting the introduction of and porting to Create would significantly reduce the number of files touched by the 'real' change in this commit, and therefore reduce noise in the commit (following the idea of "do one thing per commit" to make the code reviewable in the future).

However, if you're opposed to that, it's not a hard requirement.

To be honest I don't really see the point.

Yes, the Create refactor a simple change. The point is to remove the noise of the simple change from the complex change so that the complex change becomes visible.

It would help reviewers like myself who are less familiar with what refactoring for tail allocation involves. I couldn't read your commit and know how to do it for another class because this commit is full of noise.

Anyway, if making the code reviewable (also in the future!) like that is not important enough, I won't block this one! :)

You are right that readability and review-ability is pretty important. My apologies for making this an unreadable mess :)
Let me clean this patch a little bit (along with addressing Aaron's comments).

[riccibruno]

include/clang/AST/Expr.h

5095 ↗

(On Diff #183119)

LLVM_READONLY is a macro for __attribute__((pure)) (when supported). It is useful in some cases to give a hint to the optimizer about the behavior of a function. However in this case the definition of the function is visible in all translation units using this function. Therefore the optimizer can very well see on its own what this function is doing.

It would be perfectly possible to inspect all uses of LLVM_READONLY and only keep what is needed, but this is such a small issue that nobody bothered to do it until now.

[steveire]

include/clang/AST/Expr.h

5095 ↗

(On Diff #183119)

My point was again about commit hygiene.

You could make a commit changing this without further review, leaving this review cleaner. You could do the same with the moved friend decl. Commits which only do one thing are easier to review.

[riccibruno]

include/clang/AST/Expr.h

5095 ↗

(On Diff #183119)

Ah I see. Yes I can do this.

[riccibruno]

Cleanup the patch by factoring out the NFC changes.

@steveire This should be much clearer now.

[steveire]

Thanks, but I don't see the factored out changes in the repo. Are you going to commit those first?

Also, please factor out the introduction and use of ::Create. It seems to be unrelated to the trailing objects change.

[riccibruno]

In D57104#1369864, @steveire wrote:

Thanks, but I don't see the factored out changes in the repo. Are you going to commit those first?

Also, please factor out the introduction and use of ::Create. It seems to be unrelated to the trailing objects change.

I was planning to yes. I don't agree that the introduction of Create is unrelated. It is very much part of the changes
since the introduction of the trailing objects implies the need to do a placement new, which implies the need to
use a Create function. Also see pretty much every other patch which introduced trailing objects for the
statement/expression nodes.

[aaron.ballman]

In D57104#1369885, @riccibruno wrote:

In D57104#1369864, @steveire wrote:

Thanks, but I don't see the factored out changes in the repo. Are you going to commit those first?

Also, please factor out the introduction and use of ::Create. It seems to be unrelated to the trailing objects change.

I was planning to yes. I don't agree that the introduction of Create is unrelated. It is very much part of the changes
since the introduction of the trailing objects implies the need to do a placement new, which implies the need to
use a Create function. Also see pretty much every other patch which introduced trailing objects for the
statement/expression nodes.

+1, the changes to the Create methods are germane to this review.

This LGTM, thank you for working on it!

[steveire]

There's definitely a better possible ordering in two commits:

1. Introduce ::Create and port to it
2. Use trailing objects, taking advantage of the fact that ::Create exists.

That would make it clear in the future to other people because both commits would be cleaner, both commit messages would say what the commit does, and neither commit would have the noise of the other change.

Not splitting this commit makes it less reviewable to people who are not around today.

[steveire]

In D57104#1369985, @steveire wrote:

There's definitely a better possible ordering in two commits:

1. Introduce ::Create and port to it
2. Use trailing objects, taking advantage of the fact that ::Create exists.

That would make it clear in the future to other people because both commits would be cleaner, both commit messages would say what the commit does, and neither commit would have the noise of the other change.

Not splitting this commit makes it less reviewable to people who are not around today.

I highly recommend this 9 minute video if this is new to you or you haven't seen it before: https://youtu.be/qpdYRPL3SVE?t=103

[riccibruno]

In D57104#1369985, @steveire wrote:

There's definitely a better possible ordering in two commits:

1. Introduce ::Create and port to it
2. Use trailing objects, taking advantage of the fact that ::Create exists.

That would make it clear in the future to other people because both commits would be cleaner, both commit messages would say what the commit does, and neither commit would have the noise of the other change.

Not splitting this commit makes it less reviewable to people who are not around today.

I strongly disagree. The only reason for the Create method to exist is because we are using trailing objects.
It is not "oh nice Create exists, lets use it!". Furthermore, this is a standard pattern across the code-base and
this patch is already tiny.

[aaron.ballman]

In D57104#1369985, @steveire wrote:

There's definitely a better possible ordering in two commits:

1. Introduce ::Create and port to it
2. Use trailing objects, taking advantage of the fact that ::Create exists.

That would make it clear in the future to other people because both commits would be cleaner, both commit messages would say what the commit does, and neither commit would have the noise of the other change.

Not splitting this commit makes it less reviewable to people who are not around today.

I would find reviewing that more confusing because the two reviews are inextricably linked. There's no need for a Create() method without the other changes, and the other changes require a Create() method. I get what you mean about keeping reviews concise, but at some point you can split the reviews so much that it becomes really difficult to perform a sensible review because you get too much information in isolation.

[riccibruno]

In D57104#1370025, @aaron.ballman wrote:

In D57104#1369985, @steveire wrote:

There's definitely a better possible ordering in two commits:

1. Introduce ::Create and port to it
2. Use trailing objects, taking advantage of the fact that ::Create exists.

That would make it clear in the future to other people because both commits would be cleaner, both commit messages would say what the commit does, and neither commit would have the noise of the other change.

Not splitting this commit makes it less reviewable to people who are not around today.

I would find reviewing that more confusing because the two reviews are inextricably linked. There's no need for a Create() method without the other changes, and the other changes require a Create() method. I get what you mean about keeping reviews concise, but at some point you can split the reviews so much that it becomes really difficult to perform a sensible review because you get too much information in isolation.

+1 You can cut a commit into arbitrarily small pieces. As a silly example I could inherit from llvm::TrailingObjects in one commit.
Then remove the Stmt ** in another one and so on.

[riccibruno]

I highly recommend this 9 minute video if this is new to you or you haven't seen it before: https://youtu.be/qpdYRPL3SVE?t=103

I would like to add an additional meta-comment here, but please don't take this in a bad way. I am wondering
about the usefulness and the productivity of the "if this is new to you" in what I am assuming is a discussion
between professionals. I will be happy to address any further technical comments regarding the code itself.

[steveire]

In D57104#1370081, @riccibruno wrote:

I highly recommend this 9 minute video if this is new to you or you haven't seen it before: https://youtu.be/qpdYRPL3SVE?t=103

I would like to add an additional meta-comment here, but please don't take this in a bad way. I am wondering
about the usefulness and the productivity of the "if this is new to you" in what I am assuming is a discussion
between professionals. I will be happy to address any further technical comments regarding the code itself.

Sorry for that. It's a confusing conversation and text doesn't help! :)

Other classes have ::Create methods and if it's a justification you're looking for precedent could be it :).

My proposal is a patch which only does one thing (Pack GenericSelectionExpr) and a commit message that corresponds to that one thing. I'm not proposing 'doing half a thing' in a commit like just inheriting from llvm::TrailingObjects.

Your preference is a patch that packs GenericSelectionExpr and does something else.

I don't see how a person in the future would find the former more confusing than the latter. Someone in the future won't care that at the end of January 2019 the thing didn't already have a ::Create method. There is future-value reducing noise in commits. I know that's fuzzy and there isn't agreement on the specifics, but maybe you agree with the principle. I know many people don't agree with that principle, and many more people never use something like gitk/git log and don't see the value in granular commits (hence why someone went and gave a talk at a conference about it :)).

So, I've given that feedback and you have your LGTM. In my book, the rest is up to you! :)

[riccibruno]

In D57104#1370275, @steveire wrote:

In D57104#1370081, @riccibruno wrote:

I highly recommend this 9 minute video if this is new to you or you haven't seen it before: https://youtu.be/qpdYRPL3SVE?t=103

I would like to add an additional meta-comment here, but please don't take this in a bad way. I am wondering
about the usefulness and the productivity of the "if this is new to you" in what I am assuming is a discussion
between professionals. I will be happy to address any further technical comments regarding the code itself.

Sorry for that. It's a confusing conversation and text doesn't help! :)

I totally agree with that.

Other classes have ::Create methods and if it's a justification you're looking for precedent could be it :).

My proposal is a patch which only does one thing (Pack GenericSelectionExpr) and a commit message that corresponds to that one thing. I'm not proposing 'doing half a thing' in a commit like just inheriting from llvm::TrailingObjects.

Your preference is a patch that packs GenericSelectionExpr and does something else.

I don't think that this is accurate. This patch does one thing: pack GenericSelectionExpr. It does this by doing two logically distinct things:
1.) move some data to the bit-fields of Stmt and 2) tail-allocate the array of Stmt * and the array of TypeSourceInfo.

The addition of the Create function is part of 2) and not something distinct.

I don't see how a person in the future would find the former more confusing than the latter. Someone in the future won't care that at the end of January 2019 the thing didn't already have a ::Create method. There is future-value reducing noise in commits. I know that's fuzzy and there isn't agreement on the specifics, but maybe you agree with the principle. I know many people don't agree with that principle, and many more people never use something like gitk/git log and don't see the value in granular commits (hence why someone went and gave a talk at a conference about it :)).

I agree with you that granular commits (with good and accurate commit messages!) doing one thing are important.
I am (and I think that everyone is) a fan of (git log/blame/etc). It is indeed frustrating to do some git archaeology
and end up to a commit with no explanation.

I believe however that our disagreement here is not on whether we should have good commit messages (duh, of course we should),
nor on whether we should have granular commits (duh, of course we should too), but on whether this is a granular change.

I am arguing that it is and it seems that you are arguing that this is not the case.

So, I've given that feedback and you have your LGTM. In my book, the rest is up to you! :)

Consensus is important and I can totally miss things so I don't want to just ignore you and push ahead.

[steveire]

In D57104#1369751, @riccibruno wrote:

Cleanup the patch by factoring out the NFC changes.

@steveire This should be much clearer now.

I'd like to apply this patch locally, but I think the removed parts are not in the repo yet. Did I miss that somehow?

[riccibruno]

In D57104#1371161, @steveire wrote:

In D57104#1369751, @riccibruno wrote:

Cleanup the patch by factoring out the NFC changes.

@steveire This should be much clearer now.

I'd like to apply this patch locally, but I think the removed parts are not in the repo yet. Did I miss that somehow?

You are right that they were not available somewhere else. I just put them now in D57238. You should be able to apply D57238,
and then apply this patch on top.

[steveire]

I was just working on this this afternoon to show you the difference when it is split up. Looks like you committed this meanwhile.

While building I noticed that you introduced a -Wreorder bug in D57238, which isn't surprising given how many different things you do in that one commit :). It looks like you fixed that bug before commit though.

Thanks for the refactor!

Stephen.

cfe/trunk/include/clang/AST/Expr.h

 ///
 /// As an extension, generic selections are allowed in C++, where the following
 /// additional semantics apply:
 ///
 /// Any generic selection whose controlling expression is type-dependent or
 /// which names a dependent type in its association list is result-dependent,
 /// which means that the choice of result expression is dependent.
 /// Result-dependent generic associations are both type- and value-dependent.
-class GenericSelectionExpr : public Expr {
+class GenericSelectionExpr final
+    : public Expr,
+      private llvm::TrailingObjects<GenericSelectionExpr, Stmt *,
+                                    TypeSourceInfo *> {
   friend class ASTStmtReader;
   friend class ASTStmtWriter;
+  friend TrailingObjects;
 
   /// The number of association expressions and the index of the result
   /// expression in the case where the generic selection expression is not
   /// result-dependent. The result index is equal to ResultDependentIndex
   /// if and only if the generic selection expression is result-dependent.
   unsigned NumAssocs, ResultIndex;
   enum : unsigned {
     ResultDependentIndex = std::numeric_limits<unsigned>::max(),
     ControllingIndex = 0,
     AssocExprStartIndex = 1
   };
 
-  /// The location of the "_Generic", "default" and of the right parenthesis.
-  SourceLocation GenericLoc, DefaultLoc, RParenLoc;
+  /// The location of the "default" and of the right parenthesis.
+  SourceLocation DefaultLoc, RParenLoc;
 
-  TypeSourceInfo **AssocTypes;
-  Stmt **SubExprs;
+  // GenericSelectionExpr is followed by several trailing objects.
+  // They are (in order):
+  //
+  // * A single Stmt * for the controlling expression.
+  // * An array of getNumAssocs() Stmt * for the association expressions.
+  // * An array of getNumAssocs() TypeSourceInfo *, one for each of the
+  //   association expressions.
+  unsigned numTrailingObjects(OverloadToken<Stmt *>) const {
+    // Add one to account for the controlling expression; the remainder
+    // are the associated expressions.
+    return 1 + getNumAssocs();
+  }
 
-public:
+  unsigned numTrailingObjects(OverloadToken<TypeSourceInfo *>) const {
+    return getNumAssocs();
+  }
+
+  /// 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);
 
-  /// This constructor is used in the result-dependent case.
+  /// Build a 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);
 
-  explicit GenericSelectionExpr(EmptyShell Empty)
-      : Expr(GenericSelectionExprClass, Empty) {}
+  /// Build an empty generic selection expression for deserialization.
+  explicit GenericSelectionExpr(EmptyShell Empty, unsigned NumAssocs);
+
+public:
+  /// Create a non-result-dependent generic selection expression.
+  static GenericSelectionExpr *
+  Create(const ASTContext &Context, SourceLocation GenericLoc,
+         Expr *ControllingExpr, ArrayRef<TypeSourceInfo *> AssocTypes,
+         ArrayRef<Expr *> AssocExprs, SourceLocation DefaultLoc,
+         SourceLocation RParenLoc, bool ContainsUnexpandedParameterPack,
+         unsigned ResultIndex);
+
+  /// Create a result-dependent generic selection expression.
+  static GenericSelectionExpr *
+  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);
 
   /// 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() &&
            "Generic selection is result-dependent but getResultIndex called!");
     return ResultIndex;
   }
 
   /// Whether this generic selection is result-dependent.
   bool isResultDependent() const { return ResultIndex == ResultDependentIndex; }
 
   /// Return the controlling expression of this generic selection expression.
-  Expr *getControllingExpr() { return cast<Expr>(SubExprs[ControllingIndex]); }
+  Expr *getControllingExpr() {
+    return cast<Expr>(getTrailingObjects<Stmt *>()[ControllingIndex]);
+  }
   const Expr *getControllingExpr() const {
-    return cast<Expr>(SubExprs[ControllingIndex]);
+    return cast<Expr>(getTrailingObjects<Stmt *>()[ControllingIndex]);
   }
 
   /// Return the result expression of this controlling expression. Defined if
   /// and only if the generic selection expression is not result-dependent.
   Expr *getResultExpr() {
-    return cast<Expr>(SubExprs[AssocExprStartIndex + getResultIndex()]);
+    return cast<Expr>(
+        getTrailingObjects<Stmt *>()[AssocExprStartIndex + getResultIndex()]);
   }
   const Expr *getResultExpr() const {
-    return cast<Expr>(SubExprs[AssocExprStartIndex + getResultIndex()]);
+    return cast<Expr>(
+        getTrailingObjects<Stmt *>()[AssocExprStartIndex + getResultIndex()]);
   }
 
   ArrayRef<Expr *> getAssocExprs() const {
-    return {reinterpret_cast<Expr *const *>(SubExprs + AssocExprStartIndex),
+    return {reinterpret_cast<Expr *const *>(getTrailingObjects<Stmt *>() +
+                                            AssocExprStartIndex),
             NumAssocs};
   }
   ArrayRef<TypeSourceInfo *> getAssocTypeSourceInfos() const {
-    return {AssocTypes, NumAssocs};
+    return {getTrailingObjects<TypeSourceInfo *>(), NumAssocs};
   }
 
   Expr *getAssocExpr(unsigned i) {
-    return cast<Expr>(SubExprs[AssocExprStartIndex + i]);
+    return cast<Expr>(getTrailingObjects<Stmt *>()[AssocExprStartIndex + i]);
   }
   const Expr *getAssocExpr(unsigned i) const {
-    return cast<Expr>(SubExprs[AssocExprStartIndex + i]);
+    return cast<Expr>(getTrailingObjects<Stmt *>()[AssocExprStartIndex + i]);
   }
 
-  TypeSourceInfo *getAssocTypeSourceInfo(unsigned i) { return AssocTypes[i]; }
+  TypeSourceInfo *getAssocTypeSourceInfo(unsigned i) {
+    return getTrailingObjects<TypeSourceInfo *>()[i];
+  }
   const TypeSourceInfo *getAssocTypeSourceInfo(unsigned i) const {
-    return AssocTypes[i];
+    return getTrailingObjects<TypeSourceInfo *>()[i];
   }
 
   QualType getAssocType(unsigned i) const {
     const TypeSourceInfo *TSI = getAssocTypeSourceInfo(i);
     return TSI ? TSI->getType() : QualType();
   }
 
-  SourceLocation getGenericLoc() const { return GenericLoc; }
+  SourceLocation getGenericLoc() const {
+    return GenericSelectionExprBits.GenericLoc;
+  }
   SourceLocation getDefaultLoc() const { return DefaultLoc; }
   SourceLocation getRParenLoc() const { return RParenLoc; }
   SourceLocation getBeginLoc() const { return getGenericLoc(); }
   SourceLocation getEndLoc() const { return getRParenLoc(); }
 
   static bool classof(const Stmt *T) {
     return T->getStmtClass() == GenericSelectionExprClass;
   }
 
   child_range children() {
-    return child_range(SubExprs, SubExprs + AssocExprStartIndex + NumAssocs);
+    return child_range(getTrailingObjects<Stmt *>(),
+                       getTrailingObjects<Stmt *>() +
+                           numTrailingObjects(OverloadToken<Stmt *>()));
   }
   const_child_range children() const {
-    return const_child_range(SubExprs,
-                             SubExprs + AssocExprStartIndex + NumAssocs);
+    return const_child_range(getTrailingObjects<Stmt *>(),
+                             getTrailingObjects<Stmt *>() +
+                                 numTrailingObjects(OverloadToken<Stmt *>()));
   }
 };
 
 //===----------------------------------------------------------------------===//
 // Clang Extensions
 //===----------------------------------------------------------------------===//
 
 /// ExtVectorElementExpr - This represents access to specific elements of a

cfe/trunk/include/clang/AST/Stmt.h

class ParenListExprBitfields {
     friend class ParenListExpr;
 
     unsigned : NumExprBits;
 
     /// The number of expressions in the paren list.
     unsigned NumExprs;
   };
 
+  class GenericSelectionExprBitfields {
+    friend class ASTStmtReader;
+    friend class GenericSelectionExpr;
+
+    unsigned : NumExprBits;
+
+    /// The location of the "_Generic".
+    SourceLocation GenericLoc;
+  };
+
   class PseudoObjectExprBitfields {
     friend class ASTStmtReader; // deserialization
     friend class PseudoObjectExpr;
 
     unsigned : NumExprBits;
 
     // These don't need to be particularly wide, because they're
     // strictly limited by the forms of expressions we permit.
union {
     UnaryExprOrTypeTraitExprBitfields UnaryExprOrTypeTraitExprBits;
     ArraySubscriptExprBitfields ArraySubscriptExprBits;
     CallExprBitfields CallExprBits;
     MemberExprBitfields MemberExprBits;
     CastExprBitfields CastExprBits;
     BinaryOperatorBitfields BinaryOperatorBits;
     InitListExprBitfields InitListExprBits;
     ParenListExprBitfields ParenListExprBits;
+    GenericSelectionExprBitfields GenericSelectionExprBits;
     PseudoObjectExprBitfields PseudoObjectExprBits;
 
     // C++ Expressions
     CXXOperatorCallExprBitfields CXXOperatorCallExprBits;
     CXXBoolLiteralExprBitfields CXXBoolLiteralExprBits;
     CXXNullPtrLiteralExprBitfields CXXNullPtrLiteralExprBits;
     CXXThisExprBitfields CXXThisExprBits;
     CXXThrowExprBitfields CXXThrowExprBits;

cfe/trunk/lib/AST/Expr.cpp

void ShuffleVectorExpr::setExprs(const ASTContext &C, ArrayRef<Expr *> Exprs) {
   if (SubExprs) C.Deallocate(SubExprs);
 
   this->NumExprs = Exprs.size();
   SubExprs = new (C) Stmt*[NumExprs];
   memcpy(SubExprs, Exprs.data(), sizeof(Expr *) * Exprs.size());
 }
 
 GenericSelectionExpr::GenericSelectionExpr(
-    const ASTContext &Context, SourceLocation GenericLoc, Expr *ControllingExpr,
+    const ASTContext &, SourceLocation GenericLoc, Expr *ControllingExpr,
     ArrayRef<TypeSourceInfo *> AssocTypes, ArrayRef<Expr *> AssocExprs,
     SourceLocation DefaultLoc, SourceLocation RParenLoc,
     bool ContainsUnexpandedParameterPack, unsigned ResultIndex)
     : Expr(GenericSelectionExprClass, AssocExprs[ResultIndex]->getType(),
            AssocExprs[ResultIndex]->getValueKind(),
            AssocExprs[ResultIndex]->getObjectKind(),
            AssocExprs[ResultIndex]->isTypeDependent(),
            AssocExprs[ResultIndex]->isValueDependent(),
            AssocExprs[ResultIndex]->isInstantiationDependent(),
            ContainsUnexpandedParameterPack),
       NumAssocs(AssocExprs.size()), ResultIndex(ResultIndex),
-      AssocTypes(new (Context) TypeSourceInfo *[AssocTypes.size()]),
-      SubExprs(new (Context) Stmt *[AssocExprStartIndex + AssocExprs.size()]),
-      GenericLoc(GenericLoc), DefaultLoc(DefaultLoc), RParenLoc(RParenLoc) {
+      DefaultLoc(DefaultLoc), RParenLoc(RParenLoc) {
   assert(AssocTypes.size() == AssocExprs.size() &&
          "Must have the same number of association expressions"
          " and TypeSourceInfo!");
   assert(ResultIndex < NumAssocs && "ResultIndex is out-of-bounds!");
 
-  SubExprs[ControllingIndex] = ControllingExpr;
-  std::copy(AssocTypes.begin(), AssocTypes.end(), this->AssocTypes);
+  GenericSelectionExprBits.GenericLoc = GenericLoc;
+  getTrailingObjects<Stmt *>()[ControllingIndex] = ControllingExpr;
   std::copy(AssocExprs.begin(), AssocExprs.end(),
-            SubExprs + AssocExprStartIndex);
+            getTrailingObjects<Stmt *>() + AssocExprStartIndex);
+  std::copy(AssocTypes.begin(), AssocTypes.end(),
+            getTrailingObjects<TypeSourceInfo *>());
 }
 
 GenericSelectionExpr::GenericSelectionExpr(
     const ASTContext &Context, SourceLocation GenericLoc, Expr *ControllingExpr,
     ArrayRef<TypeSourceInfo *> AssocTypes, ArrayRef<Expr *> AssocExprs,
     SourceLocation DefaultLoc, SourceLocation RParenLoc,
     bool ContainsUnexpandedParameterPack)
     : Expr(GenericSelectionExprClass, Context.DependentTy, VK_RValue,
            OK_Ordinary,
            /*isTypeDependent=*/true,
            /*isValueDependent=*/true,
            /*isInstantiationDependent=*/true, ContainsUnexpandedParameterPack),
       NumAssocs(AssocExprs.size()), ResultIndex(ResultDependentIndex),
-      AssocTypes(new (Context) TypeSourceInfo *[AssocTypes.size()]),
-      SubExprs(new (Context) Stmt *[AssocExprStartIndex + AssocExprs.size()]),
-      GenericLoc(GenericLoc), DefaultLoc(DefaultLoc), RParenLoc(RParenLoc) {
+      DefaultLoc(DefaultLoc), RParenLoc(RParenLoc) {
   assert(AssocTypes.size() == AssocExprs.size() &&
          "Must have the same number of association expressions"
          " and TypeSourceInfo!");
 
-  SubExprs[ControllingIndex] = ControllingExpr;
-  std::copy(AssocTypes.begin(), AssocTypes.end(), this->AssocTypes);
+  GenericSelectionExprBits.GenericLoc = GenericLoc;
+  getTrailingObjects<Stmt *>()[ControllingIndex] = ControllingExpr;
   std::copy(AssocExprs.begin(), AssocExprs.end(),
-            SubExprs + AssocExprStartIndex);
+            getTrailingObjects<Stmt *>() + AssocExprStartIndex);
+  std::copy(AssocTypes.begin(), AssocTypes.end(),
+            getTrailingObjects<TypeSourceInfo *>());
+}
+
+GenericSelectionExpr::GenericSelectionExpr(EmptyShell Empty, unsigned NumAssocs)
+    : Expr(GenericSelectionExprClass, Empty), NumAssocs(NumAssocs) {}
+
+GenericSelectionExpr *GenericSelectionExpr::Create(
+    const ASTContext &Context, SourceLocation GenericLoc, Expr *ControllingExpr,
+    ArrayRef<TypeSourceInfo *> AssocTypes, ArrayRef<Expr *> AssocExprs,
+    SourceLocation DefaultLoc, SourceLocation RParenLoc,
+    bool ContainsUnexpandedParameterPack, unsigned ResultIndex) {
+  unsigned NumAssocs = AssocExprs.size();
+  void *Mem = Context.Allocate(
+      totalSizeToAlloc<Stmt *, TypeSourceInfo *>(1 + NumAssocs, NumAssocs),
+      alignof(GenericSelectionExpr));
+  return new (Mem) GenericSelectionExpr(
+      Context, GenericLoc, ControllingExpr, AssocTypes, AssocExprs, DefaultLoc,
+      RParenLoc, ContainsUnexpandedParameterPack, ResultIndex);
+}
+
+GenericSelectionExpr *GenericSelectionExpr::Create(
+    const ASTContext &Context, SourceLocation GenericLoc, Expr *ControllingExpr,
+    ArrayRef<TypeSourceInfo *> AssocTypes, ArrayRef<Expr *> AssocExprs,
+    SourceLocation DefaultLoc, SourceLocation RParenLoc,
+    bool ContainsUnexpandedParameterPack) {
+  unsigned NumAssocs = AssocExprs.size();
+  void *Mem = Context.Allocate(
+      totalSizeToAlloc<Stmt *, TypeSourceInfo *>(1 + NumAssocs, NumAssocs),
+      alignof(GenericSelectionExpr));
+  return new (Mem) GenericSelectionExpr(
+      Context, GenericLoc, ControllingExpr, AssocTypes, AssocExprs, DefaultLoc,
+      RParenLoc, ContainsUnexpandedParameterPack);
+}
+
+GenericSelectionExpr *
+GenericSelectionExpr::CreateEmpty(const ASTContext &Context,
+                                  unsigned NumAssocs) {
+  void *Mem = Context.Allocate(
+      totalSizeToAlloc<Stmt *, TypeSourceInfo *>(1 + NumAssocs, NumAssocs),
+      alignof(GenericSelectionExpr));
+  return new (Mem) GenericSelectionExpr(EmptyShell(), NumAssocs);
 }
 
 //===----------------------------------------------------------------------===//
 //  DesignatedInitExpr
 //===----------------------------------------------------------------------===//
 
 IdentifierInfo *DesignatedInitExpr::Designator::getFieldName() const {
   assert(Kind == FieldDesignator && "Only valid on a field designator");

cfe/trunk/lib/Sema/SemaExpr.cpp

for (unsigned i = 0; i < NumAssocs; ++i) {
     }
   }
   if (TypeErrorFound)
     return ExprError();
 
   // If we determined that the generic selection is result-dependent, don't
   // try to compute the result expression.
   if (IsResultDependent)
-    return new (Context) GenericSelectionExpr(
-        Context, KeyLoc, ControllingExpr, Types, Exprs, DefaultLoc, RParenLoc,
+    return GenericSelectionExpr::Create(Context, KeyLoc, ControllingExpr, Types,
+                                        Exprs, DefaultLoc, RParenLoc,
                                         ContainsUnexpandedParameterPack);
 
   SmallVector<unsigned, 1> CompatIndices;
   unsigned DefaultIndex = -1U;
   for (unsigned i = 0; i < NumAssocs; ++i) {
     if (!Types[i])
       DefaultIndex = i;
     else if (Context.typesAreCompatible(ControllingExpr->getType(),
                                         Types[i]->getType()))
Sema::CreateGenericSelectionExpr(SourceLocation KeyLoc,
   // C11 6.5.1.1p3 "If a generic selection has a generic association with a
   // type name that is compatible with the type of the controlling expression,
   // then the result expression of the generic selection is the expression
   // in that generic association. Otherwise, the result expression of the
   // generic selection is the expression in the default generic association."
   unsigned ResultIndex =
     CompatIndices.size() ? CompatIndices[0] : DefaultIndex;
 
-  return new (Context) GenericSelectionExpr(
+  return GenericSelectionExpr::Create(
       Context, KeyLoc, ControllingExpr, Types, Exprs, DefaultLoc, RParenLoc,
       ContainsUnexpandedParameterPack, ResultIndex);
 }
 
 /// getUDSuffixLoc - Create a SourceLocation for a ud-suffix, given the
 /// location of the token and the offset of the ud-suffix within it.
 static SourceLocation getUDSuffixLoc(Sema &S, SourceLocation TokLoc,
                                      unsigned Offset) {

cfe/trunk/lib/Sema/SemaExprObjC.cpp

if (ParenExpr *pe = dyn_cast<ParenExpr>(e)) {
     for (unsigned i = 0; i != n; ++i) {
       subTypes[i] = gse->getAssocTypeSourceInfo(i);
       Expr *sub = gse->getAssocExpr(i);
       if (i == gse->getResultIndex())
         sub = stripARCUnbridgedCast(sub);
       subExprs[i] = sub;
     }
 
-    return new (Context) GenericSelectionExpr(Context, gse->getGenericLoc(),
-                                              gse->getControllingExpr(),
-                                              subTypes, subExprs,
-                                              gse->getDefaultLoc(),
-                                              gse->getRParenLoc(),
-                                       gse->containsUnexpandedParameterPack(),
-                                              gse->getResultIndex());
+    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!");
     return cast<ImplicitCastExpr>(e)->getSubExpr();
   }
 }
 
 bool Sema::CheckObjCARCUnavailableWeakConversion(QualType castType,
                                                  QualType exprType) {

cfe/trunk/lib/Sema/SemaOverload.cpp

if (!GSE->isResultDependent()) {
 
       // Replace the resulting type information before rebuilding the generic
       // selection expression.
       ArrayRef<Expr *> A = GSE->getAssocExprs();
       SmallVector<Expr *, 4> AssocExprs(A.begin(), A.end());
       unsigned ResultIdx = GSE->getResultIndex();
       AssocExprs[ResultIdx] = SubExpr;
 
-      return new (Context) GenericSelectionExpr(
+      return GenericSelectionExpr::Create(
           Context, GSE->getGenericLoc(), GSE->getControllingExpr(),
           GSE->getAssocTypeSourceInfos(), AssocExprs, GSE->getDefaultLoc(),
           GSE->getRParenLoc(), GSE->containsUnexpandedParameterPack(),
           ResultIdx);
     }
     // Rather than fall through to the unreachable, return the original generic
     // selection expression.
     return GSE;

cfe/trunk/lib/Sema/SemaPseudoObject.cpp

Expr *rebuild(Expr *e) {
 
         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);
         }
 
-        return new (S.Context) GenericSelectionExpr(S.Context,
-                                                    gse->getGenericLoc(),
-                                                    gse->getControllingExpr(),
-                                                    assocTypes,
-                                                    assocs,
-                                                    gse->getDefaultLoc(),
-                                                    gse->getRParenLoc(),
-                                      gse->containsUnexpandedParameterPack(),
-                                                    resultIndex);
+        return GenericSelectionExpr::Create(
+            S.Context, gse->getGenericLoc(), gse->getControllingExpr(),
+            assocTypes, assocs, 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;
         rebuiltExpr = rebuild(rebuiltExpr);

cfe/trunk/lib/Serialization/ASTReaderStmt.cpp

 
 void ASTStmtReader::VisitBlockExpr(BlockExpr *E) {
   VisitExpr(E);
   E->setBlockDecl(ReadDeclAs<BlockDecl>());
 }
 
 void ASTStmtReader::VisitGenericSelectionExpr(GenericSelectionExpr *E) {
   VisitExpr(E);
-  E->NumAssocs = Record.readInt();
-  E->ResultIndex = Record.readInt();
-
-  E->AssocTypes = new (Record.getContext()) TypeSourceInfo *[E->NumAssocs];
-  E->SubExprs = new (Record.getContext())
-      Stmt *[GenericSelectionExpr::AssocExprStartIndex + E->NumAssocs];
-
-  E->SubExprs[GenericSelectionExpr::ControllingIndex] = Record.readSubExpr();
-  for (unsigned I = 0, N = E->getNumAssocs(); I != N; ++I) {
-    E->AssocTypes[I] = GetTypeSourceInfo();
-    E->SubExprs[GenericSelectionExpr::AssocExprStartIndex + I] =
-        Record.readSubExpr();
-  }
 
-  E->GenericLoc = ReadSourceLocation();
+  unsigned NumAssocs = Record.readInt();
+  assert(NumAssocs == E->getNumAssocs() && "Wrong NumAssocs!");
+  E->ResultIndex = Record.readInt();
+  E->GenericSelectionExprBits.GenericLoc = ReadSourceLocation();
   E->DefaultLoc = ReadSourceLocation();
   E->RParenLoc = ReadSourceLocation();
+
+  Stmt **Stmts = E->getTrailingObjects<Stmt *>();
+  // Add 1 to account for the controlling expression which is the first
+  // expression in the trailing array of Stmt *. This is not needed for
+  // the trailing array of TypeSourceInfo *.
+  for (unsigned I = 0, N = NumAssocs + 1; I < N; ++I)
+    Stmts[I] = Record.readSubExpr();
+
+  TypeSourceInfo **TSIs = E->getTrailingObjects<TypeSourceInfo *>();
+  for (unsigned I = 0, N = NumAssocs; I < N; ++I)
+    TSIs[I] = GetTypeSourceInfo();
 }
 
 void ASTStmtReader::VisitPseudoObjectExpr(PseudoObjectExpr *E) {
   VisitExpr(E);
   unsigned numSemanticExprs = Record.readInt();
   assert(numSemanticExprs + 1 == E->PseudoObjectExprBits.NumSubExprs);
   E->PseudoObjectExprBits.ResultIndex = Record.readInt();
 
case EXPR_CONVERT_VECTOR:
       S = new (Context) ConvertVectorExpr(Empty);
       break;
 
     case EXPR_BLOCK:
       S = new (Context) BlockExpr(Empty);
       break;
 
     case EXPR_GENERIC_SELECTION:
-      S = new (Context) GenericSelectionExpr(Empty);
+      S = GenericSelectionExpr::CreateEmpty(
+          Context,
+          /*NumAssocs=*/Record[ASTStmtReader::NumExprFields]);
       break;
 
     case EXPR_OBJC_STRING_LITERAL:
       S = new (Context) ObjCStringLiteral(Empty);
       break;
 
     case EXPR_OBJC_BOXED_EXPRESSION:
       S = new (Context) ObjCBoxedExpr(Empty);

cfe/trunk/lib/Serialization/ASTWriterStmt.cpp

 void ASTStmtWriter::VisitBlockExpr(BlockExpr *E) {
   VisitExpr(E);
   Record.AddDeclRef(E->getBlockDecl());
   Code = serialization::EXPR_BLOCK;
 }
 
 void ASTStmtWriter::VisitGenericSelectionExpr(GenericSelectionExpr *E) {
   VisitExpr(E);
+
   Record.push_back(E->getNumAssocs());
   Record.push_back(E->ResultIndex);
-
-  Record.AddStmt(E->getControllingExpr());
-  for (unsigned I = 0, N = E->getNumAssocs(); I != N; ++I) {
-    Record.AddTypeSourceInfo(E->getAssocTypeSourceInfo(I));
-    Record.AddStmt(E->getAssocExpr(I));
-  }
-
   Record.AddSourceLocation(E->getGenericLoc());
   Record.AddSourceLocation(E->getDefaultLoc());
   Record.AddSourceLocation(E->getRParenLoc());
+
+  Stmt **Stmts = E->getTrailingObjects<Stmt *>();
+  // Add 1 to account for the controlling expression which is the first
+  // expression in the trailing array of Stmt *. This is not needed for
+  // the trailing array of TypeSourceInfo *.
+  for (unsigned I = 0, N = E->getNumAssocs() + 1; I < N; ++I)
+    Record.AddStmt(Stmts[I]);
+
+  TypeSourceInfo **TSIs = E->getTrailingObjects<TypeSourceInfo *>();
+  for (unsigned I = 0, N = E->getNumAssocs(); I < N; ++I)
+    Record.AddTypeSourceInfo(TSIs[I]);
+
   Code = serialization::EXPR_GENERIC_SELECTION;
 }
 
 void ASTStmtWriter::VisitPseudoObjectExpr(PseudoObjectExpr *E) {
   VisitExpr(E);
   Record.push_back(E->getNumSemanticExprs());
 
   // Push the result index.  Currently, this needs to exactly match