feat: add StaticEventBus #3
Conversation
DavidWang19
force-pushed
the
master
branch
3 times, most recently
from
2ba258b to
e1fe884
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a static event bus system for the MaaUtils library with support for priority-based event handling, cancellable events, and weak pointer lifecycle management. The implementation provides both free function and member function subscription mechanisms.
- Adds
StaticEventBus.hheader with event bus implementation using templates and concepts - Implements
StaticEventBus.cppfor registry management - Includes comprehensive test suite covering basic subscriptions, priority ordering, weak pointer cleanup, and cancellable events
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| include/MaaUtils/StaticEventBus.h | Core event bus implementation with Event/CancellableEvent classes, EventStorage registry, and StaticEventManager API |
| source/StaticEventBus/StaticEventBus.cpp | Provides global EventStorageRegistry singleton implementation |
| test/StaticEventBusTest.cpp | Comprehensive test suite with 6 test cases covering all major features |
| test/CMakeLists.txt | CMake configuration for building the test executable |
| .gitignore | Comments out VS Code settings.json from ignore list |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| callback(event); | ||
| } | ||
| else { | ||
| static_assert(false, "Callback must be invocable with EventT& or const EventT&"); |
There was a problem hiding this comment.
Using static_assert(false, ...) is ill-formed in C++. It will always fail even when not instantiated. Use a type-dependent expression like static_assert(sizeof(Callable) == 0, ...) or static_assert(!sizeof(Callable*), ...) to ensure it only fires when the template is actually instantiated with an invalid type.
| static_assert(false, "Callback must be invocable with EventT& or const EventT&"); | |
| static_assert( | |
| !std::is_invocable_v<Callable&, EventT&> && | |
| !std::is_invocable_v<Callable&, const EventT&>, | |
| "Callback must be invocable with EventT& or const EventT&" | |
| ); |
| (shared_owner.get()->*callback)(event); | ||
| } | ||
| else { | ||
| static_assert(false, "Callback must be invocable with EventT& or const EventT&"); |
There was a problem hiding this comment.
Using static_assert(false, ...) is ill-formed in C++. It will always fail even when not instantiated. Use a type-dependent expression like static_assert(sizeof(Callable) == 0, ...) or static_assert(!sizeof(Callable*), ...) to ensure it only fires when the template is actually instantiated with an invalid type.
| static_assert(false, "Callback must be invocable with EventT& or const EventT&"); | |
| static_assert(sizeof(Callable) == 0, "Callback must be invocable with EventT& or const EventT&"); |
| EventStorage<EventT>& get_storage() | ||
| { | ||
| std::type_index type_idx(typeid(EventT)); | ||
| auto it = storages_.find(type_idx); | ||
| if (it == storages_.end()) { | ||
| auto storage = std::make_unique<EventStorage<EventT>>(); | ||
| auto* storage_ptr = storage.get(); | ||
| storages_[type_idx] = std::move(storage); | ||
| return *storage_ptr; | ||
| } | ||
| else { | ||
| return *static_cast<EventStorage<EventT>*>(it->second.get()); | ||
| } | ||
| } |
There was a problem hiding this comment.
The get_storage() method modifies storages_ without synchronization. If multiple threads call subscribe() or publish() concurrently with different event types, this can lead to race conditions during map modification. Consider adding mutex protection around storages_ access.
| storage.subscriptions.insert( | ||
| std::lower_bound( | ||
| storage.subscriptions.begin(), | ||
| storage.subscriptions.end(), | ||
| typename EventStorage<EventT>::Subscription { priority, false, {}, {} }), | ||
| { priority, false, std::move(wrapper), {} }); |
There was a problem hiding this comment.
The subscribe() method modifies storage.subscriptions without synchronization. Concurrent calls to subscribe() or simultaneous publish() operations on the same event type can cause data races. Consider adding mutex protection for subscription vector modifications.
| for (const auto& subscription : storage.subscriptions) { | ||
| if (subscription.has_owner && subscription.owner.expired()) { | ||
| need_erase = true; | ||
| continue; | ||
| } | ||
| subscription.callback(event); | ||
| if constexpr (std::derived_from<EventT, CancellableEvent>) { | ||
| if (event.is_cancelled()) { | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| if (need_erase) { | ||
| std::erase_if(storage.subscriptions, [](const auto& sub) { return sub.has_owner && sub.owner.expired(); }); | ||
| } |
There was a problem hiding this comment.
The publish() method iterates over and potentially modifies storage.subscriptions without synchronization. If another thread calls subscribe() during publication, this creates a data race. The iteration and cleanup operations need to be protected with the same mutex used in subscribe().
1 participant
No description provided.