Stack the created interpreters. #607

vgvassilev · 2025-05-29T10:02:21Z

This patch offers an API to switch back and forth different interpreters which can come at handy in several occasions including research into implementing threading.

Fixes #302.

github-actions

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 10 out of 14. Check the log or trigger a new build to see more.

github-actions · 2025-05-29T10:18:00Z

lib/CppInterOp/CppInterOp.cpp

-struct InterpDeleter {
-  ~InterpDeleter() = default;
-} Deleter;
+struct InterpreterInfo {


warning: class 'InterpreterInfo' defines a non-default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]

struct InterpreterInfo { ^

lib/CppInterOp/CppInterOp.cpp

github-actions · 2025-05-29T10:18:00Z

lib/CppInterOp/CppInterOp.cpp

+  // FIXME: For now we just leak the Interpreter.
+  ~InterpreterInfo() {}
+};
+static llvm::SmallVector<InterpreterInfo, 8> sInterpreters;


warning: variable 'sInterpreters' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]

static llvm::SmallVector<InterpreterInfo, 8> sInterpreters; ^

lib/CppInterOp/CppInterOp.cpp

github-actions · 2025-05-29T10:18:01Z

lib/CppInterOp/CppInterOp.cpp

-  // assert(!sInterpreter && "Interpreter already set.");
-  sInterpreter = I;
+  // assert(sInterpreters.empty() && "Interpreter already set.");
+  sInterpreters.push_back({I, /*isOwned=*/true});


warning: do not declare C-style arrays, use std::array<> instead [cppcoreguidelines-avoid-c-arrays]

auto Args = std::make_unique<const char*[]>(NumArgs + 2); ^

lib/CppInterOp/CppInterOp.cpp

github-actions · 2025-05-29T10:18:01Z

lib/CppInterOp/CppInterOp.cpp

  return I;
 }

-TInterp_t GetInterpreter() { return sInterpreter; }
+bool DeleteInterpreter(TInterp_t I /*=nullptr*/) {


warning: narrowing conversion from 'unsigned int' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]

llvm::cl::ParseCommandLineOptions(NumArgs + 1, Args.get()); ^

lib/CppInterOp/CppInterOp.cpp

codecov · 2025-05-29T10:51:42Z

Codecov Report

Attention: Patch coverage is 83.87097% with 5 lines in your changes missing coverage. Please review.

Project coverage is 77.48%. Comparing base (de1e51b) to head (c2d62be).
Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/CppInterOp/CppInterOp.cpp	83.87%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #607      +/-   ##
==========================================
+ Coverage   77.31%   77.48%   +0.17%     
==========================================
  Files           9        9              
  Lines        3685     3713      +28     
==========================================
+ Hits         2849     2877      +28     
  Misses        836      836

Files with missing lines	Coverage Δ
include/CppInterOp/CppInterOp.h	`100.00% <ø> (ø)`
lib/CppInterOp/CppInterOp.cpp	`84.87% <83.87%> (+0.21%)`	⬆️

Files with missing lines	Coverage Δ
include/CppInterOp/CppInterOp.h	`100.00% <ø> (ø)`
lib/CppInterOp/CppInterOp.cpp	`84.87% <83.87%> (+0.21%)`	⬆️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

vgvassilev · 2025-05-29T11:18:24Z

@aaronj0 and @Vipul-Cariappa, do you understand the cppyy assertion for this PR?

github-actions

clang-tidy made some suggestions

github-actions · 2025-05-29T11:49:15Z

lib/CppInterOp/CppInterOp.cpp

+};
+
+// std::deque avoids relocations and calling the dtor of InterpreterInfo.
+static llvm::ManagedStatic<std::deque<InterpreterInfo>> sInterpreters;


warning: variable 'sInterpreters' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]

static llvm::ManagedStatic<std::deque<InterpreterInfo>> sInterpreters; ^

github-actions · 2025-05-29T11:49:15Z

lib/CppInterOp/CppInterOp.cpp

-TInterp_t GetInterpreter() { return sInterpreter; }
+bool DeleteInterpreter(TInterp_t I /*=nullptr*/) {
+  if (!I) {
+    sInterpreters->pop_back();


warning: narrowing conversion from 'unsigned int' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]

llvm::cl::ParseCommandLineOptions(NumArgs + 1, Args.get()); ^

github-actions · 2025-05-29T11:49:16Z

unittests/CppInterOp/InterpreterTest.cpp

+  EXPECT_TRUE(Cpp::DeleteInterpreter(nullptr));
+  EXPECT_EQ(I2, Cpp::GetInterpreter());
+
+  auto* I4 = reinterpret_cast<void*>(static_cast<std::uintptr_t>(~0U));


warning: do not use reinterpret_cast [cppcoreguidelines-pro-type-reinterpret-cast]

auto* I4 = reinterpret_cast<void*>(static_cast<std::uintptr_t>(~0U)); ^

github-actions · 2025-05-29T11:49:16Z

unittests/CppInterOp/InterpreterTest.cpp

+  EXPECT_TRUE(Cpp::DeleteInterpreter(nullptr));
+  EXPECT_EQ(I2, Cpp::GetInterpreter());
+
+  auto* I4 = reinterpret_cast<void*>(static_cast<std::uintptr_t>(~0U));


warning: integer to pointer cast pessimizes optimization opportunities [performance-no-int-to-ptr]

auto* I4 = reinterpret_cast<void*>(static_cast<std::uintptr_t>(~0U)); ^

github-actions · 2025-05-29T11:49:16Z

unittests/CppInterOp/InterpreterTest.cpp

+  EXPECT_TRUE(Cpp::DeleteInterpreter(nullptr));
+  EXPECT_EQ(I2, Cpp::GetInterpreter());
+
+  auto* I4 = reinterpret_cast<void*>(static_cast<std::uintptr_t>(~0U));


warning: no header providing "std::uintptr_t" is directly included [misc-include-cleaner]

auto* I4 = reinterpret_cast<void*>(static_cast<std::uintptr_t>(~0U)); ^

github-actions · 2025-05-29T11:49:16Z

unittests/CppInterOp/InterpreterTest.cpp

+  EXPECT_TRUE(Cpp::Evaluate("__cplusplus") == 201703L)
+      << "Failed to activate C++17";
+
+  auto* UntrackedI = reinterpret_cast<void*>(static_cast<std::uintptr_t>(~0U));


warning: do not use reinterpret_cast [cppcoreguidelines-pro-type-reinterpret-cast]

auto* UntrackedI = reinterpret_cast<void*>(static_cast<std::uintptr_t>(~0U)); ^

github-actions · 2025-05-29T11:49:16Z

unittests/CppInterOp/InterpreterTest.cpp

+  EXPECT_TRUE(Cpp::Evaluate("__cplusplus") == 201703L)
+      << "Failed to activate C++17";
+
+  auto* UntrackedI = reinterpret_cast<void*>(static_cast<std::uintptr_t>(~0U));


warning: integer to pointer cast pessimizes optimization opportunities [performance-no-int-to-ptr]

auto* UntrackedI = reinterpret_cast<void*>(static_cast<std::uintptr_t>(~0U)); ^

Vipul-Cariappa · 2025-05-30T08:23:17Z

@aaronj0 and @Vipul-Cariappa, do you understand the cppyy assertion for this PR?

@vgvassilev, you want to look at https://github.com/compiler-research/cppyy-backend/blob/e1068109110344b63b1546c853862db9632ec737/clingwrapper/src/clingwrapper.cxx#L182C1-L212C10

We first check if an interpreter already exists; if so, we just continue using it. Otherwise, create a new interpreter.
This is required for Xeus-CPP Cppyy integration. Because xeus-cpp would create the interpreter at the start of the kernel, then when a cell with %%python is executed, cppyy is loaded. It can not create a new interpreter at that point in time, because doing so will lose all the definitions declared so far.

EDITED
I have not compiled this locally and tested it. I am suspecting this to be the cause of the assertion failure, based on the cppyy's initialization code and error message.

lib/CppInterOp/CppInterOp.cpp

github-actions

clang-tidy made some suggestions

github-actions · 2025-05-30T13:40:04Z

lib/CppInterOp/CppInterOp.cpp

-struct InterpDeleter {
-  ~InterpDeleter() = default;
-} Deleter;
+struct InterpreterInfo {


warning: class 'InterpreterInfo' defines a default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]

struct InterpreterInfo { ^

github-actions · 2025-05-30T13:40:04Z

lib/CppInterOp/CppInterOp.cpp

-TInterp_t GetInterpreter() { return sInterpreter; }
+bool DeleteInterpreter(TInterp_t I /*=nullptr*/) {
+  if (!I) {
+    sInterpreters.pop_back();


warning: narrowing conversion from 'unsigned int' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]

llvm::cl::ParseCommandLineOptions(NumArgs + 1, Args.get()); ^

unittests/CppInterOp/InterpreterTest.cpp

This patch offers an API to switch back and forth different interpreters which can come at handy in several occasions including research into implementing threading. Fixes compiler-research#302.

Vipul-Cariappa

LGTM!

github-actions bot reviewed May 29, 2025

View reviewed changes

vgvassilev force-pushed the interpreter-stack branch from 9c4439e to 4eeafdf Compare May 29, 2025 10:36

vgvassilev force-pushed the interpreter-stack branch 3 times, most recently from e01c641 to 3cb4164 Compare May 29, 2025 11:36

github-actions bot reviewed May 29, 2025

View reviewed changes

Vipul-Cariappa reviewed May 30, 2025

View reviewed changes

lib/CppInterOp/CppInterOp.cpp Outdated Show resolved Hide resolved

vgvassilev force-pushed the interpreter-stack branch 2 times, most recently from 258c884 to 5179760 Compare May 30, 2025 13:30

github-actions bot reviewed May 30, 2025

View reviewed changes

vgvassilev force-pushed the interpreter-stack branch from 5179760 to e1c2c04 Compare May 30, 2025 13:42

Stack the created interpreters.

c2d62be

This patch offers an API to switch back and forth different interpreters which can come at handy in several occasions including research into implementing threading. Fixes compiler-research#302.

vgvassilev force-pushed the interpreter-stack branch from e1c2c04 to c2d62be Compare May 30, 2025 14:35

Vipul-Cariappa approved these changes May 30, 2025

View reviewed changes

vgvassilev merged commit 27d762d into compiler-research:main May 30, 2025
78 checks passed

vgvassilev deleted the interpreter-stack branch May 30, 2025 16:36

Stack the created interpreters. #607

Stack the created interpreters. #607

Uh oh!

Conversation

vgvassilev commented May 29, 2025

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot May 29, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot May 29, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot May 29, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot May 29, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

vgvassilev commented May 29, 2025

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot May 29, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot May 29, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot May 29, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot May 29, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot May 29, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot May 29, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot May 29, 2025

Choose a reason for hiding this comment

Uh oh!

Vipul-Cariappa commented May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot May 30, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot May 30, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Vipul-Cariappa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codecov bot commented May 29, 2025 •

edited

Loading

Vipul-Cariappa commented May 30, 2025 •

edited

Loading