So as i am programming some functionalities to my game, i came across some problem that i do not understand and i seek an explaination. So i have implemented a simple event system that allows me to send communicates to different systems in my code. Everything was working fine, until i came to a problem with passing a rvalue std::string to one of my functions, please consider following code:
#include <vector>
#include <functional>
#include <iostream>
#include <unordered_map>
#include <memory>
enum class GameEvent
{
SomeEvent,
OtherEvent
};
class EventManager
{
public:
template <typename ...Args>
using EventCallback = std::function<void(Args...)>;
template <typename ...Args, typename Callback>
void addSubscriber(const GameEvent& gameEvent, Callback&& callback);
template <typename ...Args>
void notify(const GameEvent& gameEvent, Args... args);
private:
struct ListenerBase
{
virtual ~ListenerBase() = default;
};
template <typename ...Args>
struct ListenerWrapper : public ListenerBase
{
EventCallback<Args...> callback;
template <typename Callback>
ListenerWrapper(Callback&& callback_) : callback(std::forward<Callback>(callback_)) {}
virtual ~ListenerWrapper() = default;
void useCallback(Args... args)
{
callback(args...);
}
};
private:
std::unordered_map<GameEvent, std::vector<std::shared_ptr<ListenerBase>>> eventSubscribers;
};
template<typename ...Args, typename Callback>
inline void EventManager::addSubscriber(const GameEvent& gameEvent, Callback&& callback)
{
eventSubscribers[gameEvent].emplace_back(std::make_shared<ListenerWrapper<Args...>>(std::forward<Callback>(callback)));
}
template<typename ...Args>
inline void EventManager::notify(const GameEvent& gameEvent, Args ...args)
{
auto it = eventSubscribers.find(gameEvent);
if (it == eventSubscribers.end())
return;
for (const auto& callback : it->second)
{
auto castedCallback = std::static_pointer_cast<ListenerWrapper<Args...>>(callback);
castedCallback->useCallback(args...);
}
}
class SomeClass
{
public:
SomeClass(EventManager& em) : em(em) {}
void doSomething()
{
em.notify(GameEvent::SomeEvent, "abc", 6);
}
EventManager& em;
};
class SomeListener
{
public:
SomeListener(EventManager& em)
{
em.addSubscriber<const std::string&, const int&>(GameEvent::SomeEvent,
[this](const std::string& desc, const int& nr){
someEventImpl(desc, nr);
});
}
void someEventImpl(const std::string& desc, const int& nr)
{
std::cout << "Desc: " << desc << " nr: " << nr << std::endl;
}
};
int main() {
EventManager manager;
SomeListener lstr(manager);
SomeClass scs(manager);
scs.doSomething();
return 0;
}
especially the line: em.notify(GameEvent::SomeEvent, "abc", 6); in doSomething() function.
So with that plain "abc" string, when i try to run the program and it gets to this event handling function, i get that
desc is null according to visual studio compiler, and it throws an exception.
So i thought, that i will try to remove const& from the lambda funciton in SomeListener constructor, and from someEventImpl function, which came to this:
SomeListener(EventManager& em)
{
em.addSubscriber<std::string, const int&>(GameEvent::SomeEvent,
[this](std::string desc, const int& nr){
someEventImpl(desc, nr);
});
}
void someEventImpl(std::string desc, const int& nr)
{
std::cout << "Desc: " << desc << " nr: " << nr << std::endl;
}
which gave me totally different error which was 'write access violation' (or SIGSEGV according to godbolt compiler).
I wonder, why it does not work in any of these cases? And why both of these implementations give different errors? Passing an rvalue int as shown doesn't give any errors and compiles well.
In fact, a fix to this problem is pretty easy. I just have to declare a std::string and pass it to the eventManager.notify(...) function like this:
void doSomething()
{
std::string someString = "abc";
em.notify(GameEvent::SomeEvent, someString, 6);
}
And it works perfectly. I just wonder, why doesn't this rvalue string don't bind to const&, why integers do not have such a problem and why i get two different errors. Thanks in advance for explaination.
The problem is that when you're calling em.notify(GameEvent::SomeEvent, "abc", 6), Args deduces to [const char*, int].
ListenerWrapper<std::string const&, int const&>ListenerWrapper<Args...> = ListenerWrapper<const char*, int>This is undefined behavior and UBSan catches this: https://godbolt.org/z/WK4qWj3e1
Your design is extremely fragile.
When you std::static_pointer_cast, you need to downcast to exactly the type you originally stored.
However, you have taken no measures to ensure this.
In its current state, your design is no better than casting to std::any and back. Your polymorphism through ListenerBase is completely useless.
You need to somehow guarantee that each type of event has a specific, hard-coded callback signature.
Implicit conversions should also work, e.g. const char* -> std::string (which breaks your design).
This is a rough outline of something you could do:
enum class GameEvent {
SomeEvent,
OtherEvent
};
// define a callback function type for each event
template <GameEvent Event>
using CallbackType =
std::conditional_t<Event == GameEvent::SomeEvent, void(std::string_view, int),
std::conditional_t<Event == GameEvent::OtherEvent, void(int),
void>>;
// helper type
template <GameEvent Event>
struct Callbacks {
using type = CallbackType<Event>;
std::vector<std::function<type>> callbacks;
};
struct EventManager : Callbacks<GameEvent::SomeEvent>, Callbacks<GameEvent::OtherEvent> {
template <GameEvent Event, typename F>
void addSubscriber(F&& callback) {
Callbacks<Event>::callbacks.emplace_back(std::forward<F>(callback));
}
template <GameEvent Event, typename... Args>
void notify(Args... args) {
for (auto& f : Callbacks<Event>::callbacks) {
f(args...);
}
}
};
This solution is much better because it is type-safe.
In this case, GameEvent always needs to be known at compile-time. To be fair, you basically always know which game event you want to add subscribers to, or which events you want to notify, so this isn't really an issue.
You can use such an EventManager like:
EventManager e;
e.addSubscriber<GameEvent::SomeEvent>([](std::string_view s, int x) {
// ...
});
e.notify<GameEvent::SomeEvent>("foo", 123);
See live example at Compiler Explorer.
Ideally, you would also add proper constraints using std::invocable et al. to addSubscriber and notify. Otherwise, if you mess up any types involved, you would get a compiler error somewhere in emplace_back or at the call to std::function::operator()().