[CFG] [analyzer] NFC: Move construction context allocation into a helper method.

[NoQ]

Summary

It is tedious to allocate and then placement-new the construction context, so i added a tiny variadic template to improve readability.

Also fix more else-after-return thingies.

[NoQ]

I'd rather keep the helper method within the class. This allows us to keep constructors private, which is something i forgot to do originally.

[george.karpenkov]

LGTM, but would actually make even more sense as a static function for the allocator.
E.g. we have llvm::make_shared<>(), so we could also have Allocator::make_obj<...>(...)

[NoQ]

E.g. we have llvm::make_shared<>(), so we could also have Allocator::make_obj<...>(...)

I like your thinking, but that'd require me to make the allocator my friend class (:

include/clang/Analysis/ConstructionContext.h

 
 protected:
   Kind K;
 
   // Do not make public! These need to only be constructed
   // via createFromLayers().
   explicit ConstructionContext(Kind K) : K(K) {}
 
+private:
+  // A helper function for constructing an instance into a bump vector context.
+  template <typename T, typename... ArgTypes>
+  static T *create(BumpVectorContext &C, ArgTypes... Args) {
+    auto *CC = C.getAllocator().Allocate<T>();
+    return new (CC) T(Args...);
+  }
+
 public:
   /// Consume the construction context layer, together with its parent layers,
   /// and wrap it up into a complete construction context.
   static const ConstructionContext *
   createFromLayers(BumpVectorContext &C,
                    const ConstructionContextLayer *TopLayer);
 
   Kind getKind() const { return K; }
public:
   }
 };
 
 /// Represents construction into a simple local variable, eg. T var(123);.
 /// If a variable has an initializer, eg. T var = makeT();, then the final
 /// elidable copy-constructor from makeT() into var would also be a simple
 /// variable constructor handled by this class.
 class SimpleVariableConstructionContext : public VariableConstructionContext {
-public:
+  friend class ConstructionContext; // Allows to create<>() itself.
+
   explicit SimpleVariableConstructionContext(const DeclStmt *DS)
       : VariableConstructionContext(ConstructionContext::SimpleVariableKind,
                                     DS) {}
 
+public:
   static bool classof(const ConstructionContext *CC) {
     return CC->getKind() == SimpleVariableKind;
   }
 };
 
 /// Represents construction into a simple variable with an initializer syntax,
 /// with a single constructor, eg. T var = makeT();. Such construction context
 /// may only appear in C++17 because previously it was split into a temporary
 /// object constructor and an elidable simple variable copy-constructor and
 /// we were producing separate construction contexts for these constructors.
 /// In C++17 we have a single construction context that combines both.
 /// Note that if the object has trivial destructor, then this code is
 /// indistinguishable from a simple variable constructor on the AST level;
 /// in this case we provide a simple variable construction context.
 class CXX17ElidedCopyVariableConstructionContext
     : public VariableConstructionContext {
   const CXXBindTemporaryExpr *BTE;
 
-public:
+  friend class ConstructionContext; // Allows to create<>() itself.
+
   explicit CXX17ElidedCopyVariableConstructionContext(
       const DeclStmt *DS, const CXXBindTemporaryExpr *BTE)
       : VariableConstructionContext(CXX17ElidedCopyVariableKind, DS), BTE(BTE) {
     assert(BTE);
   }
 
+public:
   const CXXBindTemporaryExpr *getCXXBindTemporaryExpr() const { return BTE; }
 
   static bool classof(const ConstructionContext *CC) {
     return CC->getKind() == CXX17ElidedCopyVariableKind;
   }
 };
 
 /// Represents construction into a field or a base class within a bigger object
 /// via a constructor initializer, eg. T(): field(123) { ... }.
 class ConstructorInitializerConstructionContext : public ConstructionContext {
   const CXXCtorInitializer *I;
 
-public:
+  friend class ConstructionContext; // Allows to create<>() itself.
+
   explicit ConstructorInitializerConstructionContext(
       const CXXCtorInitializer *I)
       : ConstructionContext(ConstructionContext::ConstructorInitializerKind),
         I(I) {
     assert(I);
   }
 
+public:
   const CXXCtorInitializer *getCXXCtorInitializer() const { return I; }
 
   static bool classof(const ConstructionContext *CC) {
     return CC->getKind() == ConstructorInitializerKind;
   }
 };
 
 /// Represents immediate initialization of memory allocated by operator new,
 /// eg. new T(123);.
 class NewAllocatedObjectConstructionContext : public ConstructionContext {
   const CXXNewExpr *NE;
 
-public:
+  friend class ConstructionContext; // Allows to create<>() itself.
+
   explicit NewAllocatedObjectConstructionContext(const CXXNewExpr *NE)
       : ConstructionContext(ConstructionContext::NewAllocatedObjectKind),
         NE(NE) {
     assert(NE);
   }
 
+public:
   const CXXNewExpr *getCXXNewExpr() const { return NE; }
 
   static bool classof(const ConstructionContext *CC) {
     return CC->getKind() == NewAllocatedObjectKind;
   }
 };
 
 /// Represents a temporary object, eg. T(123), that does not immediately cross
 /// function boundaries "by value"; constructors that construct function
 /// value-type arguments or values that are immediately returned from the
 /// function that returns a value receive separate construction context kinds.
 class TemporaryObjectConstructionContext : public ConstructionContext {
   const CXXBindTemporaryExpr *BTE;
   const MaterializeTemporaryExpr *MTE;
 
-public:
+  friend class ConstructionContext; // Allows to create<>() itself.
+
   explicit TemporaryObjectConstructionContext(
       const CXXBindTemporaryExpr *BTE, const MaterializeTemporaryExpr *MTE)
       : ConstructionContext(ConstructionContext::TemporaryObjectKind),
         BTE(BTE), MTE(MTE) {
     // Both BTE and MTE can be null here, all combinations possible.
     // Even though for now at least one should be non-null, we simply haven't
     // implemented this case yet (this would be a temporary in the middle of
     // nowhere that doesn't have a non-trivial destructor).
   }
 
+public:
   /// CXXBindTemporaryExpr here is non-null as long as the temporary has
   /// a non-trivial destructor.
   const CXXBindTemporaryExpr *getCXXBindTemporaryExpr() const {
     return BTE;
   }
 
   /// MaterializeTemporaryExpr is non-null as long as the temporary is actually
   /// used after construction, eg. by binding to a reference (lifetime

 
 /// Represents a temporary object that is being immediately returned from a
 /// function by value, eg. return t; or return T(123);. In this case there is
 /// always going to be a constructor at the return site. However, the usual
 /// temporary-related bureaucracy (CXXBindTemporaryExpr,
 /// MaterializeTemporaryExpr) is normally located in the caller function's AST.
 class SimpleReturnedValueConstructionContext
     : public ReturnedValueConstructionContext {
-public:
+  friend class ConstructionContext; // Allows to create<>() itself.
+
   explicit SimpleReturnedValueConstructionContext(const ReturnStmt *RS)
       : ReturnedValueConstructionContext(
             ConstructionContext::SimpleReturnedValueKind, RS) {}
 
+public:
   static bool classof(const ConstructionContext *CC) {
     return CC->getKind() == SimpleReturnedValueKind;
   }
 };
 
 /// Represents a temporary object that is being immediately returned from a
 /// function by value, eg. return t; or return T(123); in C++17.
 /// In C++17 there is not going to be an elidable copy constructor at the
 /// return site.  However, the usual temporary-related bureaucracy (CXXBindTemporaryExpr,
 /// MaterializeTemporaryExpr) is normally located in the caller function's AST.
 /// Note that if the object has trivial destructor, then this code is
 /// indistinguishable from a simple returned value constructor on the AST level;
 /// in this case we provide a simple returned value construction context.
 class CXX17ElidedCopyReturnedValueConstructionContext
     : public ReturnedValueConstructionContext {
   const CXXBindTemporaryExpr *BTE;
 
-public:
+  friend class ConstructionContext; // Allows to create<>() itself.
+
   explicit CXX17ElidedCopyReturnedValueConstructionContext(
       const ReturnStmt *RS, const CXXBindTemporaryExpr *BTE)
       : ReturnedValueConstructionContext(
             ConstructionContext::CXX17ElidedCopyReturnedValueKind, RS),
         BTE(BTE) {
     assert(BTE);
   }
 
+public:
   const CXXBindTemporaryExpr *getCXXBindTemporaryExpr() const { return BTE; }
 
   static bool classof(const ConstructionContext *CC) {
     return CC->getKind() == CXX17ElidedCopyReturnedValueKind;
   }
 };
 
 } // end namespace clang
 
 #endif // LLVM_CLANG_ANALYSIS_CONSTRUCTIONCONTEXT_H

lib/Analysis/ConstructionContext.cpp

 const ConstructionContext *ConstructionContext::createFromLayers(
     BumpVectorContext &C, const ConstructionContextLayer *TopLayer) {
   // Before this point all we've had was a stockpile of arbitrary layers.
   // Now validate that it is shaped as one of the finite amount of expected
   // patterns.
   if (const Stmt *S = TopLayer->getTriggerStmt()) {
     if (const auto *DS = dyn_cast<DeclStmt>(S)) {
       assert(TopLayer->isLast());
-      auto *CC =
-          C.getAllocator().Allocate<SimpleVariableConstructionContext>();
-      return new (CC) SimpleVariableConstructionContext(DS);
-    } else if (const auto *NE = dyn_cast<CXXNewExpr>(S)) {
+      return create<SimpleVariableConstructionContext>(C, DS);
+    }
+    if (const auto *NE = dyn_cast<CXXNewExpr>(S)) {
       assert(TopLayer->isLast());
-      auto *CC =
-          C.getAllocator().Allocate<NewAllocatedObjectConstructionContext>();
-      return new (CC) NewAllocatedObjectConstructionContext(NE);
-    } else if (const auto *BTE = dyn_cast<CXXBindTemporaryExpr>(S)) {
+      return create<NewAllocatedObjectConstructionContext>(C, NE);
+    }
+    if (const auto *BTE = dyn_cast<CXXBindTemporaryExpr>(S)) {
       const MaterializeTemporaryExpr *MTE = nullptr;
       assert(BTE->getType().getCanonicalType()
                 ->getAsCXXRecordDecl()->hasNonTrivialDestructor());
       // For temporaries with destructors, there may or may not be
       // lifetime extension on the parent layer.
       if (const ConstructionContextLayer *ParentLayer = TopLayer->getParent()) {
         assert(ParentLayer->isLast());
         if ((MTE = dyn_cast<MaterializeTemporaryExpr>(
                  ParentLayer->getTriggerStmt()))) {
           // A temporary object which has both destruction and
           // materialization info.
-          auto *CC =
-              C.getAllocator().Allocate<TemporaryObjectConstructionContext>();
-          return new (CC) TemporaryObjectConstructionContext(BTE, MTE);
+          return create<TemporaryObjectConstructionContext>(C, BTE, MTE);
         }
         // C++17 *requires* elision of the constructor at the return site
         // and at variable initialization site, while previous standards
         // were allowing an optional elidable constructor.
         if (auto *RS = dyn_cast<ReturnStmt>(ParentLayer->getTriggerStmt())) {
           assert(!RS->getRetValue()->getType().getCanonicalType()
                     ->getAsCXXRecordDecl()->hasTrivialDestructor());
-          auto *CC =
-              C.getAllocator()
-                  .Allocate<
-                      CXX17ElidedCopyReturnedValueConstructionContext>();
-          return new (CC)
-              CXX17ElidedCopyReturnedValueConstructionContext(RS, BTE);
+          return create<CXX17ElidedCopyReturnedValueConstructionContext>(C,
+                                                                       RS, BTE);
         }
         if (auto *DS = dyn_cast<DeclStmt>(ParentLayer->getTriggerStmt())) {
           assert(!cast<VarDecl>(DS->getSingleDecl())->getType()
                       .getCanonicalType()->getAsCXXRecordDecl()
                       ->hasTrivialDestructor());
-          auto *CC =
-              C.getAllocator()
-                  .Allocate<CXX17ElidedCopyVariableConstructionContext>();
-          return new (CC) CXX17ElidedCopyVariableConstructionContext(DS, BTE);
+          return create<CXX17ElidedCopyVariableConstructionContext>(C, DS, BTE);
         }
         llvm_unreachable("Unexpected construction context with destructor!");
       }
       // A temporary object that doesn't require materialization.
-      auto *CC =
-          C.getAllocator().Allocate<TemporaryObjectConstructionContext>();
-      return new (CC)
-          TemporaryObjectConstructionContext(BTE, /*MTE=*/nullptr);
-    } else if (const auto *MTE = dyn_cast<MaterializeTemporaryExpr>(S)) {
+      return create<TemporaryObjectConstructionContext>(C, BTE, /*MTE=*/nullptr);
+    }
+    if (const auto *MTE = dyn_cast<MaterializeTemporaryExpr>(S)) {
       // If the object requires destruction and is not lifetime-extended,
       // then it must have a BTE within its MTE.
       assert(MTE->getType().getCanonicalType()
                 ->getAsCXXRecordDecl()->hasTrivialDestructor() ||
              MTE->getStorageDuration() != SD_FullExpression);
       assert(TopLayer->isLast());
-      auto *CC =
-          C.getAllocator().Allocate<TemporaryObjectConstructionContext>();
-      return new (CC) TemporaryObjectConstructionContext(nullptr, MTE);
-    } else if (const auto *RS = dyn_cast<ReturnStmt>(S)) {
+      return create<TemporaryObjectConstructionContext>(C, nullptr, MTE);
+    }
+    if (const auto *RS = dyn_cast<ReturnStmt>(S)) {
       assert(TopLayer->isLast());
-      auto *CC =
-          C.getAllocator().Allocate<SimpleReturnedValueConstructionContext>();
-      return new (CC) SimpleReturnedValueConstructionContext(RS);
+      return create<SimpleReturnedValueConstructionContext>(C, RS);
     }
+    llvm_unreachable("Unexpected construction context with statement!");
   } else if (const CXXCtorInitializer *I = TopLayer->getTriggerInit()) {
     assert(TopLayer->isLast());
-    auto *CC =
-        C.getAllocator().Allocate<ConstructorInitializerConstructionContext>();
-    return new (CC) ConstructorInitializerConstructionContext(I);
+    return create<ConstructorInitializerConstructionContext>(C, I);
   }
   llvm_unreachable("Unexpected construction context!");
 }