From 780c6ff9c89efe0465d003857923e3c340de3ca7 Mon Sep 17 00:00:00 2001
From: Wenzel Jakob <wenzel.jakob@epfl.ch>
Date: Wed, 2 Nov 2022 22:36:17 +0100
Subject: [PATCH] incorporated feedback

---
 include/pybind11/detail/internals.h   | 33 +++++++++++++++++----------
 tests/test_embed/test_interpreter.cpp | 28 ++++++++---------------
 2 files changed, 30 insertions(+), 31 deletions(-)

diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h
index 604979139..93cf246ce 100644
--- a/include/pybind11/detail/internals.h
+++ b/include/pybind11/detail/internals.h
@@ -401,6 +401,26 @@ inline void translate_local_exception(std::exception_ptr p) {
 }
 #endif
 
+inline object get_internals_state_dict() {
+    object state_dict;
+#if PY_VERSION_HEX < 0x03080000 || defined(PYPY_VERSION)
+    state_dict = reinterpret_borrow<object>(PyEval_GetBuiltins());
+#else
+#  if PY_VERSION_HEX < 0x03090000
+    PyInterpreterState *istate = _PyInterpreterState_Get();
+#else
+    PyInterpreterState *istate = PyInterpreterState_Get();
+#endif
+    if (istate)
+        state_dict = reinterpret_borrow<object>(PyInterpreterState_GetDict(istate));
+#endif
+    if (!state_dict) {
+        raise_from(PyExc_SystemError, "get_internals(): could not acquire state dictionary!");
+    }
+
+    return state_dict;
+}
+
 /// Return a reference to the current `internals` data
 PYBIND11_NOINLINE internals &get_internals() {
     internals **&internals_pp = get_internals_pp();
@@ -422,18 +442,7 @@ PYBIND11_NOINLINE internals &get_internals() {
     constexpr const char *id_cstr = PYBIND11_INTERNALS_ID;
     str id(id_cstr);
 
-    dict state_dict;
-#if PY_VERSION_HEX < 0x03080000 || defined(PYPY_VERSION)
-    state_dict = reinterpret_borrow<dict>(PyEval_GetBuiltins());
-#elif PY_VERSION_HEX < 0x03090000
-    state_dict = reinterpret_borrow<dict>(PyInterpreterState_GetDict(_PyInterpreterState_Get()));
-#else
-    state_dict = reinterpret_borrow<dict>(PyInterpreterState_GetDict(PyInterpreterState_Get()));
-#endif
-
-    if (!state_dict) {
-        pybind11_fail("get_internals(): could not acquire state dictionary!");
-    }
+    dict state_dict = get_internals_state_dict();
 
     if (state_dict.contains(id_cstr)) {
         object o = state_dict[id];
diff --git a/tests/test_embed/test_interpreter.cpp b/tests/test_embed/test_interpreter.cpp
index afcb4fc95..a363bcbee 100644
--- a/tests/test_embed/test_interpreter.cpp
+++ b/tests/test_embed/test_interpreter.cpp
@@ -168,18 +168,8 @@ TEST_CASE("There can be only one interpreter") {
     py::initialize_interpreter();
 }
 
-bool has_pybind11_internals_builtin() {
-    py::dict state_dict;
-#if PY_VERSION_HEX < 0x03080000 || defined(PYPY_VERSION)
-    state_dict = py::reinterpret_borrow<py::dict>(PyEval_GetBuiltins());
-#elif PY_VERSION_HEX < 0x03090000
-    state_dict
-        = py::reinterpret_borrow<py::dict>(PyInterpreterState_GetDict(_PyInterpreterState_Get()));
-#else
-    state_dict
-        = py::reinterpret_borrow<py::dict>(PyInterpreterState_GetDict(PyInterpreterState_Get()));
-#endif
-    return state_dict.contains(PYBIND11_INTERNALS_ID);
+bool has_pybind11_internals_state_dict() {
+    return py::detail::get_internals_state_dict().contains(PYBIND11_INTERNALS_ID);
 };
 
 bool has_pybind11_internals_static() {
@@ -190,7 +180,7 @@ bool has_pybind11_internals_static() {
 TEST_CASE("Restart the interpreter") {
     // Verify pre-restart state.
     REQUIRE(py::module_::import("widget_module").attr("add")(1, 2).cast<int>() == 3);
-    REQUIRE(has_pybind11_internals_builtin());
+    REQUIRE(has_pybind11_internals_state_dict());
     REQUIRE(has_pybind11_internals_static());
     REQUIRE(py::module_::import("external_module").attr("A")(123).attr("value").cast<int>()
             == 123);
@@ -207,10 +197,10 @@ TEST_CASE("Restart the interpreter") {
     REQUIRE(Py_IsInitialized() == 1);
 
     // Internals are deleted after a restart.
-    REQUIRE_FALSE(has_pybind11_internals_builtin());
+    REQUIRE_FALSE(has_pybind11_internals_state_dict());
     REQUIRE_FALSE(has_pybind11_internals_static());
     pybind11::detail::get_internals();
-    REQUIRE(has_pybind11_internals_builtin());
+    REQUIRE(has_pybind11_internals_state_dict());
     REQUIRE(has_pybind11_internals_static());
     REQUIRE(reinterpret_cast<uintptr_t>(*py::detail::get_internals_pp())
             == py::module_::import("external_module").attr("internals_at")().cast<uintptr_t>());
@@ -225,13 +215,13 @@ TEST_CASE("Restart the interpreter") {
               py::detail::get_internals();
               *static_cast<bool *>(ran) = true;
           });
-    REQUIRE_FALSE(has_pybind11_internals_builtin());
+    REQUIRE_FALSE(has_pybind11_internals_state_dict());
     REQUIRE_FALSE(has_pybind11_internals_static());
     REQUIRE_FALSE(ran);
     py::finalize_interpreter();
     REQUIRE(ran);
     py::initialize_interpreter();
-    REQUIRE_FALSE(has_pybind11_internals_builtin());
+    REQUIRE_FALSE(has_pybind11_internals_state_dict());
     REQUIRE_FALSE(has_pybind11_internals_static());
 
     // C++ modules can be reloaded.
@@ -253,7 +243,7 @@ TEST_CASE("Subinterpreter") {
 
         REQUIRE(m.attr("add")(1, 2).cast<int>() == 3);
     }
-    REQUIRE(has_pybind11_internals_builtin());
+    REQUIRE(has_pybind11_internals_state_dict());
     REQUIRE(has_pybind11_internals_static());
 
     /// Create and switch to a subinterpreter.
@@ -263,7 +253,7 @@ TEST_CASE("Subinterpreter") {
     // Subinterpreters get their own copy of builtins. detail::get_internals() still
     // works by returning from the static variable, i.e. all interpreters share a single
     // global pybind11::internals;
-    REQUIRE_FALSE(has_pybind11_internals_builtin());
+    REQUIRE_FALSE(has_pybind11_internals_state_dict());
     REQUIRE(has_pybind11_internals_static());
 
     // Modules tags should be gone.