[PATCH] D141215: [clang-repl] Introduce Value to capture expression results

[Vassil Vassilev via Phabricator via cfe-commits]

v.g.vassilev added a comment.

Overall it looks like we are heading in a good direction. Here are some initial comments from my partial review.



================
Comment at: clang/include/clang/Interpreter/Interpreter.h:109
+  std::list<PartialTranslationUnit> &getPTUs();
+  std::unique_ptr<llvm::Module> GenModule();
+
----------------
Do we need to expose this function to the outside world?


================
Comment at: clang/lib/Interpreter/ASTHelpers.cpp:1
+#include "ASTHelpers.h"
+
----------------
Likewise.


================
Comment at: clang/lib/Interpreter/ASTHelpers.h:1
+#ifndef LLVM_CLANG_INTERPRETER_AST_HELPERS_H
+#define LLVM_CLANG_INTERPRETER_AST_HELPERS_H
----------------
We are missing the standard llvm header preamble. Clang seems to have slight preference to using `Utils` in the name. How about renaming this file to `InterpreterUtils.h`


================
Comment at: clang/lib/Interpreter/IncrementalParser.cpp:22
 #include "clang/FrontendTool/Utils.h"
+#include "clang/Interpreter/Interpreter.h"
 #include "clang/Parse/Parser.h"
----------------
This introduces a layering violation. Can we avoid it?


================
Comment at: clang/lib/Interpreter/IncrementalParser.cpp:162
+  if (P->getCurToken().is(tok::annot_input_end)) {
+    P->ConsumeAnyToken();
     // FIXME: Clang does not call ExitScope on finalizing the regular TU, we
----------------
Why `ConsumeToken` is not sufficient for an `annot_input_end`?


================
Comment at: clang/lib/Interpreter/IncrementalParser.h:82
+
+  CodeGenerator *GetCodeGen() const;
+  std::unique_ptr<llvm::Module> GenModule();
----------------
Do we need to expose CodeGen in this patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141215