george.karpenkov requested changes to this revision.
george.karpenkov added a comment.
This revision now requires changes to proceed.

There's quite a lot of code duplication here, I think we could do better with 
that. Great job modeling semantics though!



================
Comment at: lib/Analysis/CFG.cpp:1320
+        auto *MTE = cast<MaterializeTemporaryExpr>(Child);
+        findConstructionContexts(ConstructionContextLayer::create(
+                                     cfg->getBumpVectorContext(), MTE, Layer),
----------------
There are three repeated calls to `findConstructionContexts`, with only the 
second argument changing. Perhaps a helper lambda closure would help?


================
Comment at: lib/Analysis/CFG.cpp:4959
+  const Stmt *S1 = nullptr, *S2 = nullptr, *S3 = nullptr;
+  const ConstructionContext *CC1 = nullptr;
   switch (CC->getKind()) {
----------------
begs for a comment differentiating CC from CC1


================
Comment at: lib/Analysis/CFG.cpp:5014
   }
   if (S1) {
     OS << ", ";
----------------
three blocks below could really benefit from a helper function.


================
Comment at: lib/Analysis/ConstructionContext.cpp:140
 
-      assert(TopLayer->isLast());
-      return create<TemporaryObjectConstructionContext>(C, nullptr, MTE);
+      // Handle pre-C++17 copy and move elision.
+      const CXXConstructExpr *ElidedCE = nullptr;
----------------
seems this chunk has a fair bit of code duplication vs. lines 79-99


Repository:
  rC Clang

https://reviews.llvm.org/D47616



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to