[clang-repl] Refactor locking of runtime PTU stack (NFC)

[weliveindetail]

The previous implementation seemed hacky, because it required the reader to be familiar with the internal workings of the PTU stack. The concept itself is a pragmatic solution and not very surprising. Keeping it behind a finalization call seems reasonable.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Stefan Gränitz (weliveindetail)

Changes

The previous implementation seemed hacky, because it required the reader to be familiar with the internal workings of the PTU stack. The concept itself is a pragmatic solution and not very surprising. Keeping it behind a finalization call seems reasonable.

---

Full diff: https://github.com/llvm/llvm-project/pull/84176.diff

2 Files Affected:

• (modified) clang/include/clang/Interpreter/Interpreter.h (+1)
• (modified) clang/lib/Interpreter/Interpreter.cpp (+8-4)

diff --git a/clang/include/clang/Interpreter/Interpreter.h b/clang/include/clang/Interpreter/Interpreter.h
index 292fa566ae7037..ce46aefe08a475 100644
--- a/clang/include/clang/Interpreter/Interpreter.h
+++ b/clang/include/clang/Interpreter/Interpreter.h
@@ -139,6 +139,7 @@ class Interpreter {
 
 private:
   size_t getEffectivePTUSize() const;
+  void finalizeInitPTUStack();
 
   bool FindRuntimeInterface();
 
diff --git a/clang/lib/Interpreter/Interpreter.cpp b/clang/lib/Interpreter/Interpreter.cpp
index 9f97a3c6b0be9e..e06ab45fc6fa0d 100644
--- a/clang/lib/Interpreter/Interpreter.cpp
+++ b/clang/lib/Interpreter/Interpreter.cpp
@@ -278,15 +278,14 @@ Interpreter::create(std::unique_ptr<CompilerInstance> CI) {
   if (Err)
     return std::move(Err);
 
+  // Runtimes contain essential definitions that are irrevocable. Lock their
+  // stack of initial PTUs to make them unavailable for undo.
   auto PTU = Interp->Parse(Runtimes);
   if (!PTU)
     return PTU.takeError();
+  Interp->finalizeInitPTUStack();
 
   Interp->ValuePrintingInfo.resize(4);
-  // FIXME: This is a ugly hack. Undo command checks its availability by looking
-  // at the size of the PTU list. However we have parsed something in the
-  // beginning of the REPL so we have to mark them as 'Irrevocable'.
-  Interp->InitPTUSize = Interp->IncrParser->getPTUs().size();
   return std::move(Interp);
 }
 
@@ -343,6 +342,11 @@ const ASTContext &Interpreter::getASTContext() const {
   return getCompilerInstance()->getASTContext();
 }
 
+void Interpreter::finalizeInitPTUStack() {
+  assert(!InitPTUSize && "We only do this once");
+  InitPTUSize = IncrParser->getPTUs().size();
+}
+
 size_t Interpreter::getEffectivePTUSize() const {
   std::list<PartialTranslationUnit> &PTUs = IncrParser->getPTUs();
   assert(PTUs.size() >= InitPTUSize && "empty PTU list?");

[vgvassilev]

This looks reasonable to me. Is it worth to add some tests?

[weliveindetail]

Yes sure! It requires access to the interface, but IMHO that makes a lot of sense anyway.

[vgvassilev]

Wouldn’t using the public interpreter::Undo indirectly achieve the same effect? If it errors out we will know the size of the builtin PTUs.

[weliveindetail]

Yes, that's right. In fact, we could just not expose it right now and instead keep this a NFC. Then we don't need a new test either.

[weliveindetail]

Rebased and reduced to refactoring NFC

[weliveindetail]

Yes, that's right. In fact, we could just not expose it right now and instead keep this a NFC. Then we don't need a new test either.