argentite added inline comments.

================
Comment at: clang/include/clang/Interpreter/Interpreter.h:62
   const CompilerInstance *getCompilerInstance() const;
-  const llvm::orc::LLJIT *getExecutionEngine() const;
+  llvm::Expected<llvm::orc::LLJIT &> GetExecutionEngine();
+
----------------
sgraenitz wrote:
> Do we really want these functions capitalized? IIRC in the clang code-base 
> they are used to highlight core API functions. It looks like these two 
> functions are only used as helpers inside `Interpreter` itself.
I guess we can keep them without capitalizing.


================
Comment at: clang/lib/Interpreter/Interpreter.cpp:223
 
+llvm::Error Interpreter::CreateExecutor() {
+  const clang::TargetInfo &TI =
----------------
sgraenitz wrote:
> Factoring out this function looks like an independent change. Is it related 
> to the load-library command in any way?
The `IncrementalExecutor` was being created on the first call to `Execute()`. 
It is created lazily for `-fsyntax-only` support. It holds the Execution Engine 
that we need for loading the library. So we need to be able to create it in 
`getExecutionEngine()` as well because a library can be loaded before any code 
is executed.


================
Comment at: clang/tools/clang-repl/ClangRepl.cpp:126
       }
+      if (Line->rfind(R"(%lib )", 0) == 0) {
+        if (auto Err = Interp->LoadDynamicLibrary(Line->data() + 5)) {
----------------
sgraenitz wrote:
> I see this is analog to existing code, but is there a good reason for using 
> the multi-line string syntax here?
Yeah I am confused as well. But I tried to maintain the style.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141824

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to