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

[Vassil Vassilev via Phabricator via cfe-commits]

v.g.vassilev added inline comments.


================
Comment at: clang/include/clang/Interpreter/Interpreter.h:20
+#include "clang/Interpreter/PartialTranslationUnit.h"
+#include "clang/Interpreter/Value.h"
 
----------------
We can forward declare `Value`.


================
Comment at: clang/include/clang/Interpreter/Interpreter.h:39
+class Parser;
+class CodeGenerator;
 class CompilerInstance;
----------------
We probably do not need these forward declarations.


================
Comment at: clang/include/clang/Interpreter/Interpreter.h:101
+
+  Parser &getParser() const;
+
----------------
This seems to be not needed as well.


================
Comment at: clang/include/clang/Interpreter/Interpreter.h:106
+  Expr *RuntimeAllocInterface = nullptr;
+  Expr *RuntimeCopyArrInterface = nullptr;
+
----------------
Maybe these can go in a separate data structure called `ValuePrintingInfo` or similar.


================
Comment at: clang/include/clang/Interpreter/Interpreter.h:119
+
+  std::unique_ptr<llvm::Module> GenModule();
+
----------------
We should not need this interface.


================
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
----------------
junaire wrote:
> v.g.vassilev wrote:
> > Why `ConsumeToken` is not sufficient for an `annot_input_end`?
> No, we can't. `ConsumeToken` can't eat special tokens. See https://github.com/llvm/llvm-project/blob/fc6e91fe8184129d2395b79ce42f4495b95b0d0d/clang/include/clang/Parse/Parser.h#L494-L501
Ok, thanks!


================
Comment at: clang/lib/Interpreter/Interpreter.cpp:35
 
+#include "ASTHelpers.h"
+#include "clang/Sema/Lookup.h"
----------------
This seems to be pointing to the old filename.


================
Comment at: clang/lib/Interpreter/Interpreter.cpp:226
     return std::move(Err);
+  llvm::cantFail(Interp->ParseAndExecute(Runtimes));
+  // FIXME: This is a ugly hack. Undo command checks its availability by looking
----------------
We should avoid `cantFail` here and propagate the error as we do a few lines above.


================
Comment at: clang/lib/Interpreter/Value.cpp:1
+#include "clang/Interpreter/Value.h"
+#include "clang/AST/ASTContext.h"
----------------
We are missing the llvm preamble here as well.


================
Comment at: clang/lib/Parse/ParseStmt.cpp:551
+    ConsumeAnyToken();
+    setPrettyPrintMode();
+  } else {
----------------
I think the cleaner solution here is to remove the global flag and pass it `handleExprStmt` and extend `Sema::ActOnExprStmt`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141215