[PATCH] D126682: [Interpreter][ClangRepl] Implement undo command

[Vassil Vassilev via Phabricator via cfe-commits]

v.g.vassilev added a comment.

Thanks for working on this. Here are some more comments.



================
Comment at: clang/include/clang/Interpreter/Interpreter.h:72
 
+  void Restore(PartialTranslationUnit &PTU);
+
----------------
I am not sure if that's the best function. We 


================
Comment at: clang/lib/Interpreter/IncrementalParser.cpp:281
 
+void IncrementalParser::Restore(PartialTranslationUnit &PTU) {
+  TranslationUnitDecl *MostRecentTU = PTU.TUPart;
----------------
I think we can merge this function with `Undo` where we can conditionally check if we have a llvm::Module to recover.


================
Comment at: clang/lib/Interpreter/Interpreter.cpp:273
+    llvm::Error Err = llvm::Error::success();
+    IncrExecutor = std::make_unique<IncrementalExecutor>(*TSCtx, Err, Triple);
+
----------------
I am not sure how feasible is this usecase but in `-fsyntax-only` mode we can still need undo. That means somebody created an Interpreter instance for frontend-only operations, such as lookups or template instantiations. In that case I think it also would make sense to support the undo operation.


================
Comment at: clang/lib/Interpreter/Interpreter.cpp:278
+  }
+  if (N > IncrExecutor->getAvailableModuleSize())
+    return llvm::make_error<llvm::StringError>("Operation failed, "
----------------
In some cases we can have PTU's that do not generate code. For example, `define A 1` will not produce a corresponding llvm::Module. I think here we should count the size of the PartialTranslationUnitDecls.


================
Comment at: clang/test/Interpreter/code-undo.cpp:7
+// RUN: cat %s | clang-repl | FileCheck %s
+extern "C" int printf(const char *, ...);
+int x1 = 42;
----------------
Can you add here something like `int j = 0` and print its value after the `undo`. This way we will make sure we do not undo too much.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126682