[PATCH] D127284: [clang-repl] Support statements on global scope in incremental mode.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

This is looking good to me. No further non-trivial concerns on my side.



================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:6165
+  // Stop squashing the top-level stmts into a single function.
+  if (CurCGF && !CXXGlobalInits.back()->getName().startswith("__stmts__")) {
+    CurCGF->FinishFunction(D->getEndLoc());
----------------
Instead of a name comparison, can you check whether `CXXGlobalInits.back() == CurCGF->CurFn`?


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:6172
+    // void __stmts__N(void)
+    std::string Name = "__stmts__" + llvm::utostr(CXXGlobalInits.size());
+    FunctionArgList Args;
----------------
Maybe add a TODO to ask the ABI name mangler to pick a name?


================
Comment at: clang/lib/Parse/ParseDecl.cpp:5395
+  // FIXME: What do we do if we get something in Stmts?
+  assert(!Stmts.size() && "Unsupported multiple stmt!");
+
----------------
I guess this happens in some pragma situations? Can you put a real diagnostic in here rather than an assert?


================
Comment at: clang/lib/Parse/Parser.cpp:1032-1034
+    // FIXME: Remove the incremental processing pre-condition and verify clang
+    // still can pass its test suite, which will harden
+    // `isDeclarationStatement`.
----------------
Have you tried this with the latest version of the patch? Can the FIXME be removed?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127284/new/

https://reviews.llvm.org/D127284