[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

A more general comment: You have to `std::move` your `llvm::Error` variables when the result is `llvm::Expected` (otherwise this won't compile).



================
Comment at: clang/include/clang/Interpreter/Transaction.h:1
+//===--- Transaction.h - Incremental Compilation and Execution---*- C++ -*-===//
+//
----------------
Could this whole file just be part of `IncrementalParser.h` which is the only user ? `clang::Transaction` seems anyway a bit of a generic name, so maybe this could become `clang::IncrementalParser::Transaction` then.


================
Comment at: clang/include/clang/Interpreter/Transaction.h:10
+// This file defines utilities tracking the incrementally processed pieces of
+// code
+//
----------------
missing `.`.


================
Comment at: clang/include/clang/Parse/Parser.h:2403
   }
-
+private:
   /// isForInitDeclaration - Disambiguates between a declaration or an
----------------
This doesn't seem to be needed for anything?


================
Comment at: clang/lib/Interpreter/IncrementalParser.cpp:175
+  memcpy(MBStart, input.data(), InputSize);
+  memcpy(MBStart + InputSize, "\n", 2);
+
----------------
I think overwriting the \0 in the buffer isn't necessary. So `MBStart[InputSize] = '\n';` should be enough.


================
Comment at: clang/lib/Interpreter/Interpreter.cpp:43
+#include "llvm/Support/Host.h"
+//
+
----------------
I think the comment here/above and some of the empty lines aren't really needed here.


================
Comment at: clang/lib/Interpreter/Interpreter.cpp:140
+
+  if (std::find(ClangArgv.begin(), ClangArgv.end(), " -x") == ClangArgv.end()) {
+    // We do C++ by default; append right after argv[0] if no "-x" given
----------------
`llvm::find(ClangArgv, " -x")`


================
Comment at: clang/tools/clang-repl/CMakeLists.txt:3
+#  ${LLVM_TARGETS_TO_BUILD}
+#  Option
+  Support
----------------
Commented out by mistake?


================
Comment at: clang/unittests/CodeGen/IncrementalProcessingTest.cpp:56-59
+  std::vector<std::string> ClangArgs = {"-Xclang", "-emit-llvm-only"};
+  std::vector<const char *> ClangArgv(ClangArgs.size());
+  std::transform(ClangArgs.begin(), ClangArgs.end(), ClangArgv.begin(),
+                 [](const std::string &s) -> const char * { return s.data(); });
----------------



================
Comment at: clang/unittests/Interpreter/InterpreterTest.cpp:69
+
+  EXPECT_DEATH((void)Interp->Parse("int var1 = 42;"), "");
+}
----------------
I think that's usually used for testing asserts but here it's an invalid memory access (which might even work out just if the stars align correctly).

What about either:
1. Shutting down clang-repl cleanly after we hit a diagnostic.
2. Making an assert that we can't codegen a TU that already had any error diagnostic (in which case you can surround it with the following):

```
#if !defined(NDEBUG) && GTEST_HAS_DEATH_TEST
```


Repository:
  rC Clang

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

https://reviews.llvm.org/D96033