-
-
Notifications
You must be signed in to change notification settings - Fork 44
✨ Refactor QDMI Devices and Fix Singleton Handling #1347
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
67e0402
2812858
ab883c3
f9097cd
8553ccb
19e699b
39d47cb
6620171
e30ea78
332c3e1
204ad85
6e4500a
9a9a734
6c1b974
078679a
9aa2ba8
72faea0
ac0c32d
d759783
8a99129
2e2d440
2f2bb5a
080b813
aa441bc
bde5874
eac1c9e
8c638ca
8edd823
302c199
a0b3ee3
6f768be
e6eb3e1
4e458a6
a3bdab6
3cd5dba
0ac07d2
ca6f8ba
cdc1cd2
5049d82
e9d2e62
0fe2fae
18bf0fe
d3a6f13
e03baf8
89fdf07
fd9c562
e68f5e8
9d88c5a
b4dfb5a
801a2ef
433f9c4
8dccb47
0f920cb
7393c68
40405cb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,97 @@ | ||
| /* | ||
| * Copyright (c) 2023 - 2025 Chair for Design Automation, TUM | ||
| * Copyright (c) 2025 Munich Quantum Software Company GmbH | ||
| * All rights reserved. | ||
| * | ||
| * SPDX-License-Identifier: MIT | ||
| * | ||
| * Licensed under the MIT License | ||
| */ | ||
|
|
||
| #pragma once | ||
|
|
||
| /** @file | ||
| * @brief The generic MQT QDMI device implementation that specific devices | ||
| * inherit from. | ||
| */ | ||
|
|
||
| #include <cassert> | ||
| #include <memory> | ||
| #include <mutex> | ||
|
|
||
| // Define a macro for hidden visibility, which works on GCC and Clang. | ||
| #if defined(__GNUC__) || defined(__clang__) | ||
| #define MQT_HIDDEN __attribute__((visibility("hidden"))) | ||
| #else | ||
| #define MQT_HIDDEN | ||
| #endif | ||
|
|
||
| namespace qdmi { | ||
| template <class ConcreteType> class SingletonDevice { | ||
| // NOLINTNEXTLINE(cppcoreguidelines-owning-memory) | ||
| MQT_HIDDEN inline static std::shared_ptr<ConcreteType>* instance = nullptr; | ||
| // NOLINTNEXTLINE(cppcoreguidelines-owning-memory) | ||
| MQT_HIDDEN inline static std::mutex* mutex = new std::mutex; | ||
|
|
||
|
Comment on lines
+32
to
+35
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: # First, find and read the Device.hpp file to see the full SingletonDevice implementation
find . -name "Device.hpp" -path "*/qdmi/devices/base/*" | head -5Repository: munich-quantum-toolkit/core Length of output: 118 🏁 Script executed: # Read the full Device.hpp file to understand the complete SingletonDevice implementation
cat -n ./include/mqt-core/qdmi/devices/base/Device.hppRepository: munich-quantum-toolkit/core Length of output: 3837 🏁 Script executed: # Search for calls to SingletonDevice::get() to see if there are production callers
# and if they provide any guards or assumptions
rg "\.get\(\)" --type cpp -B 2 -A 2 | head -80Repository: munich-quantum-toolkit/core Length of output: 5521 🏁 Script executed: # Search more specifically for SingletonDevice class usage and get() calls
# First find all files that use SingletonDevice
rg "SingletonDevice" --type cpp -l | head -20Repository: munich-quantum-toolkit/core Length of output: 248 🏁 Script executed: # Check one concrete device implementation to see the pattern
cat -n ./include/mqt-core/qdmi/devices/dd/Device.hpp | head -50Repository: munich-quantum-toolkit/core Length of output: 1665 🏁 Script executed: # Search for Device::get() or similar get() calls in QDMI device code
rg "Device::get|device.*get\(\)" include/mqt-core/qdmi/ src/qdmi/ --type cpp -B 2 -A 2Repository: munich-quantum-toolkit/core Length of output: 5176 🏁 Script executed: # Check one of the Device.cpp implementation files to see the full context
cat -n ./src/qdmi/devices/dd/Device.cpp | head -100Repository: munich-quantum-toolkit/core Length of output: 4468 🏁 Script executed: # Look at the specific get() calls and their exception handling
rg -A 10 "Device::get\(\)->" src/qdmi/devices/dd/Device.cpp | head -60Repository: munich-quantum-toolkit/core Length of output: 2340 🏁 Script executed: # Let me check if there are any test files that verify get() behavior when device is not initialized
find . -path "*/test/*" -name "*device*test*.cpp" -o -path "*/test/*" -name "*Device*test*.cpp" | grep -i qdmi | head -5Repository: munich-quantum-toolkit/core Length of output: 166 🏁 Script executed: # Check one test file to see initialization patterns
cat -n ./test/qdmi/devices/dd/device_status_test.cpp | head -80Repository: munich-quantum-toolkit/core Length of output: 3380 🏁 Script executed: # Check the SessionGuard implementation to see if it calls initialize()
cat -n ./test/qdmi/dd/helpers/test_utils.hpp | head -100Repository: munich-quantum-toolkit/core Length of output: 140 🏁 Script executed: # Find test_utils.hpp
find . -name "test_utils.hpp" -path "*/qdmi/*"Repository: munich-quantum-toolkit/core Length of output: 116 🏁 Script executed: # Read test_utils.hpp to see SessionGuard implementation
cat -n ./test/qdmi/devices/dd/helpers/test_utils.hppRepository: munich-quantum-toolkit/core Length of output: 2608 🏁 Script executed: # Find the SessionGuard implementation
rg "SessionGuard::" test/qdmi/devices/dd/helpers/ -A 5Repository: munich-quantum-toolkit/core Length of output: 1020 Fix The [[nodiscard]] static auto get() -> std::shared_ptr<ConcreteType> {
const std::scoped_lock lock(*mutex);
assert(*instance != nullptr &&
"Device not initialized. Call `initialize()` first.");
return *instance;
}Should be changed to: [[nodiscard]] static auto get() -> std::shared_ptr<ConcreteType> {
const std::scoped_lock lock(*mutex);
if (instance == nullptr || !*instance) {
throw std::runtime_error(
"Device not initialized. Call `initialize()` before `get()`.");
}
return *instance;
}This matches the documented error handling pattern already expected by production code callers (lines 57-64, 71-85, 90-95 in Device.cpp which catch 🤖 Prompt for AI Agents |
||
| protected: | ||
| /// @brief Protected constructor to enforce the singleton pattern. | ||
| SingletonDevice() = default; | ||
| // Allow std::make_shared to access the protected constructor. | ||
| friend class std::shared_ptr<ConcreteType>; | ||
|
|
||
| public: | ||
| // Delete move constructor and move assignment operator. | ||
| SingletonDevice(SingletonDevice&&) = delete; | ||
| SingletonDevice& operator=(SingletonDevice&&) = delete; | ||
| // Delete copy constructor and assignment operator to enforce singleton. | ||
| SingletonDevice(const SingletonDevice&) = delete; | ||
| SingletonDevice& operator=(const SingletonDevice&) = delete; | ||
|
|
||
| /// @brief Destructor for the SingletonDevice class. | ||
| virtual ~SingletonDevice() = default; | ||
|
|
||
| /** | ||
| * @brief Initializes the singleton instance. | ||
| * @details Must be called before `get()`. | ||
| */ | ||
| static void initialize() { | ||
| const std::scoped_lock lock(*mutex); | ||
| if (instance == nullptr) { | ||
| // NOLINTNEXTLINE(cppcoreguidelines-owning-memory) | ||
| instance = new std::shared_ptr<ConcreteType>(new ConcreteType); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * @brief Destroys the singleton instance. | ||
| * @details After this call, `get()` must not be called until a new | ||
| * `initialize()` call. Any existing shared_ptr will keep the object alive | ||
| * until they go out of scope. | ||
| */ | ||
| static void finalize() { | ||
| const std::scoped_lock lock(*mutex); | ||
| if (instance != nullptr) { | ||
| // Reset the shared_ptr to release the managed object. | ||
| *instance = nullptr; | ||
| // Delete the shared_ptr object itself. | ||
| // NOLINTNEXTLINE(cppcoreguidelines-owning-memory) | ||
| delete instance; | ||
| instance = nullptr; | ||
| } | ||
| // Do NOT delete the static mutex. The mutex is intentionally leaked to | ||
| // avoid static deinitialization issues (cf. static deinitialization order | ||
| // fiasco) | ||
| } | ||
|
|
||
| /** | ||
| * @brief Get the singleton instance of the device. | ||
| * @return A shared pointer to the device instance. | ||
| */ | ||
| [[nodiscard]] static auto get() -> std::shared_ptr<ConcreteType> { | ||
| const std::scoped_lock lock(*mutex); | ||
| assert(*instance != nullptr && | ||
| "Device not initialized. Call `initialize()` first."); | ||
| return *instance; | ||
| } | ||
| }; | ||
| } // namespace qdmi | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| # Copyright (c) 2023 - 2025 Chair for Design Automation, TUM | ||
| # Copyright (c) 2025 Munich Quantum Software Company GmbH | ||
| # All rights reserved. | ||
| # | ||
| # SPDX-License-Identifier: MIT | ||
| # | ||
| # Licensed under the MIT License | ||
|
|
||
| add_subdirectory(base) | ||
| add_subdirectory(na) | ||
| add_subdirectory(dd) | ||
| add_subdirectory(sc) | ||
|
|
||
| # add to list of MQT core targets | ||
| set(MQT_CORE_TARGETS | ||
| ${MQT_CORE_TARGETS} | ||
| PARENT_SCOPE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick | 🔵 Trivial
Consider avoiding raw owning pointers for
instanceandmutexYou intentionally leak the
mutexto sidestep static deinitialization order, and manageinstancevia a raw pointer:This works, but it is non‑idiomatic and easy to misuse. A cleaner alternative is to rely on function‑local statics for initialization order and avoid manual
new/delete:and then use
instanceStorage()/instanceMutex()ininitialize(),finalize(), andget(). That eliminates raw owning pointers and the intentional leak while still avoiding the static deinit‑order fiasco.Not mandatory for correctness once
get()is fixed, but it would simplify reasoning and static‑analysis tooling.Also applies to: 32-35
🤖 Prompt for AI Agents