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

[Vassil Vassilev via Phabricator via cfe-commits]

v.g.vassilev added a comment.

@teemperor, thanks for the hints.



================
Comment at: clang/include/clang/Interpreter/Transaction.h:1
+//===--- Transaction.h - Incremental Compilation and Execution---*- C++ -*-===//
+//
----------------
teemperor wrote:
> 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.
The intent is to expose the Transaction class and if I move it in the `IncrementalParser` I will have to expose the `IncrementalParser.h`. Would it make sense to move it as part of the Interpreter object? Eg. `clang::Interpreter::Transaction`?


================
Comment at: clang/include/clang/Parse/Parser.h:2403
   }
-
+private:
   /// isForInitDeclaration - Disambiguates between a declaration or an
----------------
teemperor wrote:
> This doesn't seem to be needed for anything?
Good catch -- indeed that's a leftover of a previous experiments.


================
Comment at: clang/lib/Interpreter/Interpreter.cpp:43
+#include "llvm/Support/Host.h"
+//
+
----------------
teemperor wrote:
> I think the comment here/above and some of the empty lines aren't really needed here.
Yeah, the include section surrounded with `//` is to keep track of the includes required by `IncrementalCompilerBuilder::create`. I am not sure where this code should live... 

Ideally, I'd like to have it in a place where it can be included by other clients which require compiler instance set up for incremental compilation.


================
Comment at: clang/tools/clang-repl/CMakeLists.txt:3
+#  ${LLVM_TARGETS_TO_BUILD}
+#  Option
+  Support
----------------
teemperor wrote:
> Commented out by mistake?
Does not seem to be required. I will remove it for now...


================
Comment at: clang/unittests/Interpreter/InterpreterTest.cpp:69
+
+  EXPECT_DEATH((void)Interp->Parse("int var1 = 42;"), "");
+}
----------------
teemperor wrote:
> 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
> ```
Would it make sense just to not test that case?


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

https://reviews.llvm.org/D96033