[AST] Refactor PredefinedExpr

[riccibruno]

Summary

Make the following changes to PredefinedExpr:

1. Move PredefinedExpr below StringLiteral so that it can use its definition.
2. Rename IdentType to IdentKind to be more in line with clang's conventions.
3. Move the location and the IdentKind into the newly available space of the bit-fields of Stmt.
4. Only store the function name when needed. When parsing all of Boost, of the 1357 PredefinedExpr 919 have no function name.

[rjmccall]

include/clang/AST/Expr.h

1687

"always" would be clearer than "in fact".

include/clang/AST/Stmt.h

279

Since you're changing this around anyway, please make it a "kind" rather than a "type". Even if it's just the field name for now, it's progress.

283

Not sure why this is abbreviated.

lib/AST/Expr.cpp

470

!= nullptr is clearer.

[riccibruno]

Address rjmcall's comments. In particular use "Kind" instead of "Type" and propagate
the change to the users. Change the title to "[AST] Refactor PredefinedExpr" since this
patch is doing various changes to PredefinedExpr.

[rjmccall]

LGTM.

include/clang/AST/Stmt.h

279

Thank you! That was more than I expected, but it's definitely better this way.

include/clang/AST/Expr.h

 #include "clang/Basic/SyncScope.h"
 #include "clang/Basic/TypeTraits.h"
 #include "llvm/ADT/APFloat.h"
 #include "llvm/ADT/APSInt.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/AtomicOrdering.h"
 #include "llvm/Support/Compiler.h"
+#include "llvm/Support/TrailingObjects.h"
 
 namespace clang {
   class APValue;
   class ASTContext;
   class BlockDecl;
   class CXXBaseSpecifier;
   class CXXMemberCallExpr;
   class CXXOperatorCallExpr;
const_child_range children() const {
     return const_child_range(const_child_iterator(), const_child_iterator());
   }
 
   friend TrailingObjects;
   friend class ASTStmtReader;
   friend class ASTStmtWriter;
 };
 
-/// [C99 6.4.2.2] - A predefined identifier such as __func__.
-class PredefinedExpr : public Expr {
-public:
-  enum IdentType {
-    Func,
-    Function,
-    LFunction, // Same as Function, but as wide string.
-    FuncDName,
-    FuncSig,
-    LFuncSig, // Same as FuncSig, but as as wide string
-    PrettyFunction,
-    /// The same as PrettyFunction, except that the
-    /// 'virtual' keyword is omitted for virtual member functions.
-    PrettyFunctionNoVirtual
-  };
-
-private:
-  SourceLocation Loc;
-  IdentType Type;
-  Stmt *FnName;
-
-public:
-  PredefinedExpr(SourceLocation L, QualType FNTy, IdentType IT,
-                 StringLiteral *SL);
-
-  /// Construct an empty predefined expression.
-  explicit PredefinedExpr(EmptyShell Empty)
-      : Expr(PredefinedExprClass, Empty), Loc(), Type(Func), FnName(nullptr) {}
-
-  IdentType getIdentType() const { return Type; }
-
-  SourceLocation getLocation() const { return Loc; }
-  void setLocation(SourceLocation L) { Loc = L; }
-
-  StringLiteral *getFunctionName();
-  const StringLiteral *getFunctionName() const {
-    return const_cast<PredefinedExpr *>(this)->getFunctionName();
-  }
-
-  static StringRef getIdentTypeName(IdentType IT);
-  static std::string ComputeName(IdentType IT, const Decl *CurrentDecl);
-
-  SourceLocation getBeginLoc() const LLVM_READONLY { return Loc; }
-  SourceLocation getEndLoc() const LLVM_READONLY { return Loc; }
-
-  static bool classof(const Stmt *T) {
-    return T->getStmtClass() == PredefinedExprClass;
-  }
-
-  // Iterators
-  child_range children() { return child_range(&FnName, &FnName + 1); }
-  const_child_range children() const {
-    return const_child_range(&FnName, &FnName + 1);
-  }
-
-  friend class ASTStmtReader;
-};
-
 /// Used by IntegerLiteral/FloatingLiteral to store the numeric without
 /// leaking memory.
 ///
 /// For large floats/integers, APFloat/APInt will allocate memory from the heap
 /// to represent these numbers.  Unfortunately, when we use a BumpPtrAllocator
 /// to allocate IntegerLiteral/FloatingLiteral nodes the memory associated with
 /// the APFloat/APInt values will never get freed. APNumericStorage uses
 /// ASTContext's allocator for memory allocation.
public:
   child_range children() {
     return child_range(child_iterator(), child_iterator());
   }
   const_child_range children() const {
     return const_child_range(const_child_iterator(), const_child_iterator());
   }
 };
 
+/// [C99 6.4.2.2] - A predefined identifier such as __func__.
+class PredefinedExpr final
+    : public Expr,
+      private llvm::TrailingObjects<PredefinedExpr, Stmt *> {
+  friend class ASTStmtReader;
+  friend TrailingObjects;
+
+  // PredefinedExpr is optionally followed by a single trailing
+  // "Stmt *" for the predefined identifier. It is present if and only if
+  // hasFunctionName() is true and is in fact a "StringLiteral *".
+
+public:
+  enum IdentType {
+    Func,
+    Function,
+    LFunction, // Same as Function, but as wide string.
+    FuncDName,
+    FuncSig,
+    LFuncSig, // Same as FuncSig, but as as wide string
+    PrettyFunction,
+    /// The same as PrettyFunction, except that the
+    /// 'virtual' keyword is omitted for virtual member functions.
+    PrettyFunctionNoVirtual
+  };
+
+private:
+  PredefinedExpr(SourceLocation L, QualType FNTy, IdentType IT,
+                 StringLiteral *SL);
+
+  explicit PredefinedExpr(EmptyShell Empty, bool HasFunctionName);
+
+  bool hasFunctionName() const { return PredefinedExprBits.HasFnName; }
+
+  void setFunctionName(StringLiteral *SL) {
+    assert(hasFunctionName() &&
+           "This PredefinedExpr has no storage for a function name!");
+    *getTrailingObjects<Stmt *>() = SL;
+  }
+
+public:
+  /// Create a PredefinedExpr.
+  static PredefinedExpr *Create(const ASTContext &Ctx, SourceLocation L,
+                                QualType FNTy, IdentType IT, StringLiteral *SL);
+
+  /// Create an empty PredefinedExpr.
+  static PredefinedExpr *CreateEmpty(const ASTContext &Ctx,
+                                     bool HasFunctionName);
+
+  IdentType getIdentType() const {
+    return static_cast<IdentType>(PredefinedExprBits.Type);
+  }
+
+  SourceLocation getLocation() const { return PredefinedExprBits.Loc; }
+  void setLocation(SourceLocation L) { PredefinedExprBits.Loc = L; }
+
+  StringLiteral *getFunctionName() {
+    return hasFunctionName()
+               ? static_cast<StringLiteral *>(*getTrailingObjects<Stmt *>())
+               : nullptr;
+  }
+
+  const StringLiteral *getFunctionName() const {
+    return hasFunctionName()
+               ? static_cast<StringLiteral *>(*getTrailingObjects<Stmt *>())
+               : nullptr;
+  }
+
+  static StringRef getIdentTypeName(IdentType IT);
+  static std::string ComputeName(IdentType IT, const Decl *CurrentDecl);
+
+  SourceLocation getBeginLoc() const { return getLocation(); }
+  SourceLocation getEndLoc() const { return getLocation(); }
+
+  static bool classof(const Stmt *T) {
+    return T->getStmtClass() == PredefinedExprClass;
+  }
+
+  // Iterators
+  child_range children() {
+    return child_range(getTrailingObjects<Stmt *>(),
+                       getTrailingObjects<Stmt *>() + hasFunctionName());
+  }
+};
+
 /// ParenExpr - This represents a parethesized expression, e.g. "(1)".  This
 /// AST node is only formed if full location information is requested.
 class ParenExpr : public Expr {
   SourceLocation L, R;
   Stmt *Val;
 public:
   ParenExpr(SourceLocation l, SourceLocation r, Expr *val)
     : Expr(ParenExprClass, val->getType(),

include/clang/AST/Stmt.h

class ExprBitfields {
     unsigned ObjectKind : 3;
     unsigned TypeDependent : 1;
     unsigned ValueDependent : 1;
     unsigned InstantiationDependent : 1;
     unsigned ContainsUnexpandedParameterPack : 1;
   };
   enum { NumExprBits = 17 };
 
+  class PredefinedExprBitfields {
+    friend class ASTStmtReader;
+    friend class PredefinedExpr;
+
+    unsigned : NumExprBits;
+
+    /// The type of this PredefinedExpr. One of the enumeration values
+    /// in PredefinedExpr::IdentType.
+    unsigned Type : 4;
+
+    /// True if this PredefinedExpr has a trailing "StringLiteral *"
+    /// for the predefined identifier.
+    unsigned HasFnName : 1;
+
+    /// The location of this PredefinedExpr.
+    SourceLocation Loc;
+  };
+
   class CharacterLiteralBitfields {
     friend class CharacterLiteral;
 
     unsigned : NumExprBits;
 
     unsigned Kind : 3;
   };
 
union {
     ForStmtBitfields ForStmtBits;
     GotoStmtBitfields GotoStmtBits;
     ContinueStmtBitfields ContinueStmtBits;
     BreakStmtBitfields BreakStmtBits;
     ReturnStmtBitfields ReturnStmtBits;
 
     // Expressions
     ExprBitfields ExprBits;
+    PredefinedExprBitfields PredefinedExprBits;
     CharacterLiteralBitfields CharacterLiteralBits;
     FloatingLiteralBitfields FloatingLiteralBits;
     UnaryExprOrTypeTraitExprBitfields UnaryExprOrTypeTraitExprBits;
     DeclRefExprBitfields DeclRefExprBits;
     CastExprBitfields CastExprBits;
     CallExprBitfields CallExprBits;
     ExprWithCleanupsBitfields ExprWithCleanupsBits;
     PseudoObjectExprBitfields PseudoObjectExprBits;

lib/AST/ASTImporter.cpp

ExpectedStmt ASTNodeImporter::VisitPredefinedExpr(PredefinedExpr *E) {
   if (!Imp)
     return Imp.takeError();
 
   SourceLocation ToBeginLoc;
   QualType ToType;
   StringLiteral *ToFunctionName;
   std::tie(ToBeginLoc, ToType, ToFunctionName) = *Imp;
 
-  return new (Importer.getToContext()) PredefinedExpr(
-      ToBeginLoc, ToType, E->getIdentType(), ToFunctionName);
+  return PredefinedExpr::Create(Importer.getToContext(), ToBeginLoc, ToType,
+                                E->getIdentType(), ToFunctionName);
 }
 
 ExpectedStmt ASTNodeImporter::VisitDeclRefExpr(DeclRefExpr *E) {
   auto Imp = importSeq(
       E->getQualifierLoc(), E->getTemplateKeywordLoc(), E->getDecl(),
       E->getLocation(), E->getType());
   if (!Imp)
     return Imp.takeError();

lib/AST/Expr.cpp

SourceLocation DeclRefExpr::getEndLoc() const {
   return getNameInfo().getEndLoc();
 }
 
 PredefinedExpr::PredefinedExpr(SourceLocation L, QualType FNTy, IdentType IT,
                                StringLiteral *SL)
     : Expr(PredefinedExprClass, FNTy, VK_LValue, OK_Ordinary,
            FNTy->isDependentType(), FNTy->isDependentType(),
            FNTy->isInstantiationDependentType(),
-           /*ContainsUnexpandedParameterPack=*/false),
-      Loc(L), Type(IT), FnName(SL) {}
-
-StringLiteral *PredefinedExpr::getFunctionName() {
-  return cast_or_null<StringLiteral>(FnName);
+           /*ContainsUnexpandedParameterPack=*/false) {
+  PredefinedExprBits.Type = IT;
+  assert((getIdentType() == IT) &&
+         "IdentType do not fit in PredefinedExprBitfields!");
+  bool HasFunctionName = !!SL;
+  PredefinedExprBits.HasFnName = HasFunctionName;
+  PredefinedExprBits.Loc = L;
+  if (HasFunctionName)
+    setFunctionName(SL);
+}
+
+PredefinedExpr::PredefinedExpr(EmptyShell Empty, bool HasFunctionName)
+    : Expr(PredefinedExprClass, Empty) {
+  PredefinedExprBits.HasFnName = HasFunctionName;
+}
+
+PredefinedExpr *PredefinedExpr::Create(const ASTContext &Ctx, SourceLocation L,
+                                       QualType FNTy, IdentType IT,
+                                       StringLiteral *SL) {
+  bool HasFunctionName = !!SL;
+  void *Mem = Ctx.Allocate(totalSizeToAlloc<Stmt *>(HasFunctionName),
+                           alignof(PredefinedExpr));
+  return new (Mem) PredefinedExpr(L, FNTy, IT, SL);
+}
+
+PredefinedExpr *PredefinedExpr::CreateEmpty(const ASTContext &Ctx,
+                                            bool HasFunctionName) {
+  void *Mem = Ctx.Allocate(totalSizeToAlloc<Stmt *>(HasFunctionName),
+                           alignof(PredefinedExpr));
+  return new (Mem) PredefinedExpr(EmptyShell(), HasFunctionName);
 }
 
 StringRef PredefinedExpr::getIdentTypeName(PredefinedExpr::IdentType IT) {
   switch (IT) {
   case Func:
     return "__func__";
   case Function:
     return "__FUNCTION__";

lib/Sema/SemaExpr.cpp

if (IT == PredefinedExpr::LFunction || IT == PredefinedExpr::LFuncSig) {
       ResTy = Context.adjustStringLiteralBaseType(Context.CharTy.withConst());
       ResTy = Context.getConstantArrayType(ResTy, LengthI, ArrayType::Normal,
                                            /*IndexTypeQuals*/ 0);
       SL = StringLiteral::Create(Context, Str, StringLiteral::Ascii,
                                  /*Pascal*/ false, ResTy, Loc);
     }
   }
 
-  return new (Context) PredefinedExpr(Loc, ResTy, IT, SL);
+  return PredefinedExpr::Create(Context, Loc, ResTy, IT, SL);
 }
 
 ExprResult Sema::ActOnPredefinedExpr(SourceLocation Loc, tok::TokenKind Kind) {
   PredefinedExpr::IdentType IT;
 
   switch (Kind) {
   default: llvm_unreachable("Unknown simple primary expr!");
   case tok::kw___func__: IT = PredefinedExpr::Func; break; // [C99 6.4.2.2]

lib/Serialization/ASTReaderStmt.cpp

void ASTStmtReader::VisitExpr(Expr *E) {
   E->setValueKind(static_cast<ExprValueKind>(Record.readInt()));
   E->setObjectKind(static_cast<ExprObjectKind>(Record.readInt()));
   assert(Record.getIdx() == NumExprFields &&
          "Incorrect expression field count");
 }
 
 void ASTStmtReader::VisitPredefinedExpr(PredefinedExpr *E) {
   VisitExpr(E);
+  bool HasFunctionName = Record.readInt();
+  E->PredefinedExprBits.HasFnName = HasFunctionName;
+  E->PredefinedExprBits.Type = Record.readInt();
   E->setLocation(ReadSourceLocation());
-  E->Type = (PredefinedExpr::IdentType)Record.readInt();
-  E->FnName = cast_or_null<StringLiteral>(Record.readSubExpr());
+  if (HasFunctionName)
+    E->setFunctionName(cast<StringLiteral>(Record.readSubExpr()));
 }
 
 void ASTStmtReader::VisitDeclRefExpr(DeclRefExpr *E) {
   VisitExpr(E);
 
   E->DeclRefExprBits.HasQualifier = Record.readInt();
   E->DeclRefExprBits.HasFoundDecl = Record.readInt();
   E->DeclRefExprBits.HasTemplateKWAndArgsInfo = Record.readInt();
case STMT_GCCASM:
       S = new (Context) GCCAsmStmt(Empty);
       break;
 
     case STMT_MSASM:
       S = new (Context) MSAsmStmt(Empty);
       break;
 
     case STMT_CAPTURED:
-      S = CapturedStmt::CreateDeserialized(Context,
-                                           Record[ASTStmtReader::NumStmtFields]);
+      S = CapturedStmt::CreateDeserialized(
+          Context, Record[ASTStmtReader::NumStmtFields]);
       break;
 
     case EXPR_PREDEFINED:
-      S = new (Context) PredefinedExpr(Empty);
+      S = PredefinedExpr::CreateEmpty(
+          Context,
+          /*HasFunctionName*/ Record[ASTStmtReader::NumExprFields]);
       break;
 
     case EXPR_DECL_REF:
       S = DeclRefExpr::CreateEmpty(
         Context,
         /*HasQualifier=*/Record[ASTStmtReader::NumExprFields],
         /*HasFoundDecl=*/Record[ASTStmtReader::NumExprFields + 1],
         /*HasTemplateKWAndArgsInfo=*/Record[ASTStmtReader::NumExprFields + 2],

lib/Serialization/ASTWriterStmt.cpp

void ASTStmtWriter::VisitExpr(Expr *E) {
   Record.push_back(E->isInstantiationDependent());
   Record.push_back(E->containsUnexpandedParameterPack());
   Record.push_back(E->getValueKind());
   Record.push_back(E->getObjectKind());
 }
 
 void ASTStmtWriter::VisitPredefinedExpr(PredefinedExpr *E) {
   VisitExpr(E);
-  Record.AddSourceLocation(E->getLocation());
+
+  bool HasFunctionName = !!E->getFunctionName();
+  Record.push_back(HasFunctionName);
   Record.push_back(E->getIdentType()); // FIXME: stable encoding
+  Record.AddSourceLocation(E->getLocation());
+  if (HasFunctionName)
     Record.AddStmt(E->getFunctionName());
   Code = serialization::EXPR_PREDEFINED;
 }
 
 void ASTStmtWriter::VisitDeclRefExpr(DeclRefExpr *E) {
   VisitExpr(E);
 
   Record.push_back(E->hasQualifier());
   Record.push_back(E->getDecl() != E->getFoundDecl());