[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:39
+class Parser;
+class CodeGenerator;
 class CompilerInstance;
----------------
v.g.vassilev wrote:
> We probably do not need these forward declarations.
Looks like they came back.


================
Comment at: clang/include/clang/Interpreter/Interpreter.h:67
   create(std::unique_ptr<CompilerInstance> CI);
+  ASTContext &getASTContext() const;
   const CompilerInstance *getCompilerInstance() const;
----------------
We should not need this. `CompileDtorCall` can probably do `RD->getASTContext()` instead.


================
Comment at: clang/include/clang/Interpreter/Interpreter.h:104
+
+  Expr *SynthesizeExpr(clang::Expr *E);
+
----------------
Let's make this file static.


================
Comment at: clang/include/clang/Interpreter/Value.h:1
+//===--- Interpreter.h - Incremental Compiation and Execution---*- C++ -*-===//
+//
----------------
`Value.h` instead of `Interpreter.h`


================
Comment at: clang/include/clang/Interpreter/Value.h:17
+
+#include "llvm/Support/Compiler.h"
+#include <cassert>
----------------
We should probably remove this include as it seems unneeded.


================
Comment at: clang/lib/Interpreter/IncrementalASTConsumer.cpp:37
+
+void IncrementalASTConsumer::Transform(DeclGroupRef &DGR) {
+  for (Decl *D : DGR) {
----------------
Let's inline this function in its caller.


================
Comment at: clang/lib/Interpreter/IncrementalASTConsumer.cpp:43
+      // Flip the flag.
+      TSD->setValuePrinting(false);
+    }
----------------
Do we need to do this? I'd prefer to keep some history in `TSD`.


================
Comment at: clang/lib/Interpreter/IncrementalASTConsumer.h:25
+
+class IncrementalASTConsumer final : public ASTConsumer {
+  Interpreter &Interp;
----------------
Let's move this class in `IncrementalParser.cpp`. I don't think we need a new file here just yet.


================
Comment at: clang/lib/Interpreter/IncrementalASTConsumer.h:34
+  bool HandleTopLevelDecl(DeclGroupRef DGR) override final;
+  void HandleTranslationUnit(ASTContext &Ctx) override final;
+  static bool classof(const clang::ASTConsumer *) { return true; }
----------------
We should make all interfaces pass through as all of them are important events that codegen needs to know about.


================
Comment at: clang/lib/Interpreter/IncrementalParser.h:19
 #include "clang/AST/GlobalDecl.h"
-
+#include "clang/Parse/Parser.h"
 #include "llvm/ADT/ArrayRef.h"
----------------
We don't need this include if we remove `getParser` which we do not need.


================
Comment at: clang/lib/Interpreter/IncrementalParser.h:87
+private:
+  CodeGenerator *GetCodeGen() const;
 };
----------------
That should not be needed with the new implementation in this patch.


================
Comment at: clang/lib/Interpreter/Interpreter.cpp:350
   std::list<PartialTranslationUnit> &PTUs = IncrParser->getPTUs();
-  if (N > PTUs.size())
+  if (N + InitPTUSize > PTUs.size())
     return llvm::make_error<llvm::StringError>("Operation failed. "
----------------
I'd propose `IncrParser->getPTUs()` to return the list starting from `InitPTUSize`. That should solve the issue you see.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141215