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