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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
