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

Vassil Vassilev via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Sat Jun 25 08:54:17 PDT 2022

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.

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list