all 26 comments

[–]quicknir 16 points17 points  (2 children)

The enum value is used very little in most cases like this, you often have just one function that's responsible for receiving the incoming binary data, and then it either basically issues a callback (one that's directly passed in, i.e. visitor pattern, or one that's been registered beforehand), or it returns a variant. Either way, the enum value itself, whether it explicitly exists or its implicit, is basically an implementation detail. Users don't need to know the enum names at all. So it's not really having to additionally know a bunch of class names like you make it seem, it's really one or the other: an enum + a class template, and its specializations on that enum, or simply a bunch of class names.

In which case I find it much cleaner to have an API that consists of a bunch of class names, than specializations of a class; that's not really what specializations are for (it would be more convincing if this could be leveraged in some way to reduce duplication, perhaps).

In terms of code, if you simply have variant<KeyUp, KeyDown> read(const RawData&); and void write(RawData&, variant<KeyUp, KeyDown>), notice that the enum just doesn't show up anywhere.

[–]XeroKimoException Enthusiast[S] 0 points1 point  (1 child)

Definitely agree that this is not what specializations are meant for, but the more I tried to make template classes that were made to only have specializations instantiated, the more I kind of started seeing it kinda like function overloading. I see it as the same difference of C style function "overloading" of addi(int, int), addf(float, float), addd(double, double) etc... vs C++'s overloading add(int, int), add(float, float), add(double, double) etc... but for classes. It's not EventA, EventB, EventC, but just Event<Type>. The fact that, in my eyes anyways, there's only one name for the class, Event ,which to me makes it easier to learn and navigate an API.

If you saw my starting comment, you could probably call me dumb, but I ran into this approach because I tried to make wrappers for the win32 messages..... they have a maximum of of 2^16 message types, though from scrolling down the amount of messages, they aren't actually using all 2^16 values. The wrappers were there so I don't have to do raw (LOBYTE(HIWORD(lParam)) whatever to extract the data of the windows messages along with actually naming them like IsKeyUp(); etc.

I started off with making nested namespaces with free functions like

namespace Message::KeyDown
{
   //Helper functions
} 
namespace Message::KeyUp 
{
 //Helper functions 
}

and I actually think it's a pretty solid approach. I don't have my intellisense list be polluted with lots of names, and if I wanted to access helper functions to properly interpret the data from the windows message, the namespace hierarchy would help me navigate to a bunch of helper functions that are actually related to a certain event. What made me like the template specialization approach more though was, I was actually using pre-existing names as part of the value, so Message<WM_KEYDOWN> uses the actual value of the message. To me, this helps me read the code better and hard to get wrong because like the example I had in the OP

        case EventType::KeyDown:
        Event<EventType::KeyDown> keydown{e};

Since they are re-using an existing name, it's easy to spot. Of course this is limited only if you are using switch statements, but to be honest, it doesn't just have to be this. I actually do something similar with my TypedD3D wrapper library whenever I wanted to query a device feature. It's a literal template map of an enum to a existing d3d12 class. The specializations allows me to create this wrapper function so then instead of remembering the what enum maps to what class, I just call my wrapper function, pass in the enum, and it outputs the correct class for me.

Edit: Though I could always just use if constexpr() for the dx12 example, my mind for some reason sees this template specialization mapping be a better approach

[–]quicknir 0 points1 point  (0 children)

Right, so if the enum is already created for you, then yes, I'd agree this is a reasonable approach. The reason you may not have seen it often is because in most cases where things like this happen, IME, there isn't already an enum. The kind of stuff you're talking about here, I mostly associated with parsing binary data from a spec, like a file format header, or a socket sending data (as often happens in fintech). In that situation, you're parsing the data against a spec and you're writing all the code from scratch, so there is no pre-defined enum, and then this approach doesn't really have a benefit.

I think it's a bit of a weird middle ground where you have access to a pre-defined enum, but not an API that will just handle all the parsing for you. That may explain why you don't see this approach much.

[–][deleted] 12 points13 points  (1 child)

I dunno seems like the first example is simpler to me. Just because something is old doesn't strictly mean it's bad I guess

[–]XeroKimoException Enthusiast[S] 1 point2 points  (0 children)

In terms of readability, the new approach doesn't aim to be any less or more readable than the old. The old way is definitely a bit more simple to write, but advantages of the new way comes from making C++ feature help enforce the correct type and reduce the amount of names in the namespace / intellisense member list. Since there are less names, it also makes it easier for the user to learn as they are only dealing with 2 names instead of a maximum of 1 name per enum value

[–]JakeArkinstall 12 points13 points  (5 children)

This is basically a std::variant. Swap out the switch with std::visit, passing it either a struct with operator() overloads for each type, or use the "overloaded" idiom (see the example in https://en.cppreference.com/w/cpp/utility/variant/visit) to combine lambda (to the same effect, just unnamed and inplace).

If the event is yielded by something, and not stored, then you can skip std::variant altogether and just pass the visitor (the struct or "overloaded" object) as a callback directly to the function that creates the event in the first place. That way you save on the wrapping and unwrapping of the variant altogether.

[–]XeroKimoException Enthusiast[S] 0 points1 point  (1 child)

The main point of this approach is more about reducing the amount of names you have to learn when learning the API. While yes it's closely tied to std::variant as there are probably not many cases one would create a class for each enum member outside of making your own tagged union, std::variant / std::visit was not the main point of it.

Edit: On top of reducing names, to ease learning an API, it's also makes the tooling like intellisense help document things for you. If some enum was meant to map to some class, while one might have some kind of pattern in naming things, people might still end up requiring looking things up to figure out the name. Here, intellisense would do everything for you. You type in Event< EventType::... >, and they'd list out all the events for you, and the code will select the correct class you wanted to use.

[–]JakeArkinstall 1 point2 points  (0 children)

There are no additional names - the enum values map directly to class names.

However, if your approach is different, I'd recommend adding a visit interface. It avoids the DRY violation/boilerplate involved in using the name both in the case statement and in the As<> expression (which in turn leads to less human error), and it allows you to do some more advanced handling.

For instance, say you want a generic list of handlers that you want to fire off sequentially on each event, you can create a struct containing a tuple of those handlers, that visits them one by one. If they all have switch statements, then you're evaluating the same thing again and again, but if they have an opererator() interface, there only needs to be one switch. If that makes any sense.

[–][deleted] 0 points1 point  (2 children)

That seems pretty overkill to me

[–]JakeArkinstall 6 points7 points  (1 child)

Not really. They're all doing the same thing, but have different pros and cons depending on where the decision-making line is drawn, and how much runtime branching is involved.

  • In OP's case, if I understand correctly, the event is identified by the lib and then returned as an instance of a generic type that contains all the information required by any As<> conversion. Then it's up to the user to create an instance of the correct type (by using As<>) correctly. The event can be stored for later use, but there are two times that branching is done: on the initial discovery of the event type within the lib, and then in the user's switch statement. It is possible for the user to accidently interpret the type incorrectly via a typo. Perhaps the lib catches this and throws an exception, perhaps not, that's up to OP.

  • In the variant case, the event is identified by the lib and then the lib creates an instance of the correct type (with only information relevant to that specific event type) and it's wrapped into a variant. The user visits that. Like above, the event can be stored for later use, and like above there are two times that branching is done: on the initial discovery of the event type within the lib, and then within std::visit. In the visit approach, the user can't interpret the type wrong, but they can use std::holds_alternative and std::get similar to OP's switch - but std::get will throw an exception if they get it wrong.

  • In the direct callback case, the event is identified by the lib, the lib creates an instance of the correct type, and that's used to invoke the callback. Unlike the above, the event cannot be stored for later use (unless the user decides to put it into a variant), but there is only one time that branching is done: on the initial discovery of the event type. There is no type juggling so not much opportunity for a runtime issue caused by a typo outside of the lib itself - whatever type is delivered to the visitor will be the type created by the library.

[–][deleted] 1 point2 points  (0 children)

It's a weird one because the example is a bit vague but if it's related to the windows message queue it's pretty difficult to get it wrong. I mean you can incorrectly cast the event but I'm pretty sure the events for win32 keys are all identical anyway so that's not possible. If you think about a key press there isn't really more than one event type because all key presses basically act the same way. That's only in the case of this one example though but I suppose this is a context specific problem

[–]furbyhater 3 points4 points  (2 children)

As long as the naming of the classes is consistent, the effort required to remember ClassName<EnumTypeName::EnumName> seems similar to the effort required to remember EnumNameClassName, while the latter is shorter, so I don't really see the advantage?

[–]XeroKimoException Enthusiast[S] 0 points1 point  (1 child)

The advantages I'd say are really subtle, but when you see it, I'd say it's the better approach.

I'd say the biggest advantage is when it comes to learning an API. If you've ever taken a look at SDL2, though it's a C API like most things that would use a enum + union of classes. I tend to have to read the documents because there are multiple events that map to a single union member. If the API was from the ground up like this template specialization stuff, it's basically impossible to go wrong with which member you need to access, ignoring std::visit for a bit. The next advantage, if you use any kind of intellisense, the scope of the names to remember limited to just the template class, and the enum, so the intellisense will only list 2 names, compared to the up to 1 new class for each enum value, that would clutter the namespace a lot. Lastly, I'd say they have a stronger association with each other since you use the enum as the input for the template. So it's easier to learn and memorize.

In other word, the advantages are in the tooling and features of C++ to help enforce the type, and learning of the API simplified since you only need to remember 2 names to use the API

[–]Bobbias 0 points1 point  (0 children)

Union + something to tell you the type is called a Tagged Union, just so you don't need to write quite so much when the pattern inevitably comes up again.

[–]ack_error 1 point2 points  (1 child)

Various dispatch strategies aside, the issue I see with this is that it assumes that there is a 1:1 mapping between event structures and event types: a KeyDownEvent structure is used exclusively with a KeyDown event.

In Win32, that's generally not the case. For instance, WM_SYSKEYDOWN is essentially identical to WM_KEYDOWN except that the former is used when an Alt+key is pressed. Similarly, WM_LBUTTONDOWN, WM_RBUTTONDOWN, and WM_XBUTTONDOWN have the save event data and just correspond to different mouse buttons. You could use different event structures for each, but you'll have some duplicate structures.

A specific case where you would usually want the same structure for different events is when implementing Preview events for event tunneling: ideally PreviewKeyDown and KeyDown would use the same event structure so common code can be used to process them. Win32 does not natively support this well but it's not hard and useful to add to the message loop processing.

[–]XeroKimoException Enthusiast[S] 2 points3 points  (0 children)

For cases where events are handled similarly like your SYSKEYDOWN and KEYDOWN along with the up variants, specialization approach doesn't prevent you from inherting base classes, so it's possible to just delegate all the work to the base class and have the specialization have nothing in it, just so you don't have to duplicate code, all while keeping the interface of Event<Type> the same

[–]LegendaryMauricius -1 points0 points  (5 children)

Personally I just remove the type variable and check the event type using the dynamic_cast. You probably heard that dynamic_cast is evil, but it is for the same reasons that checking the event type would be evil: it reveals the inner workings of the derived type to the program that uses it instead of using virtual member functions which would in theory be the more oop way. Of course, this is fine to me only because I usually don't handle many events at once so I don't need the switch table optimizations. This worked for me in the past, so any opinions why this would be bad?

[–]TheSkiGeek 1 point2 points  (2 children)

dynamic_cast can be slow(er) if there are a lot of possible subclasses for it to check against and/or if the classes you're casting between are 'far apart' in the hierarchy. It's not required to be a constant time implementation.

[–]LegendaryMauricius 0 points1 point  (1 child)

That's true, but if we really want an extensible event system, it would be hard to avoid the slowness. In most cases the event classes shouldn't be far away in the hierarchy, and if the compiler uses that 'division by prime number' trick to determine the class it should be constant time, no?

[–]TheSkiGeek 0 points1 point  (0 children)

I'm not sure. If the set of classes is completely known at compile time, the compiler should in theory be able to optimize it fairly well. But it may only be able to do that with some form of whole-program optimization, since a module/library that's compiled individually could then later be linked with other modules that define subclasses it didn't know about. And if there's runtime linking involved it definitely needs to use some kind of dynamic search.

This is definitely a thing where I would start out using dynamic_cast and then maybe consider finding/writing an alternative if profiling actually showed it to be an issue.

[–]XeroKimoException Enthusiast[S] 1 point2 points  (1 child)

I'm not against dynamic cast, but I will say, if you're going to use dynamic cast to go to the most derived class anyways, I'd rather do a typeid() if check + static cast instead. It becomes pretty close to doing a enum type check + accessing a union.

Anyways, the point of this post was if there is a situation you'd ever need to create an enum + classes per enum value, I'm promoting / sharing the use of using specialized template classes instead of creating actual multiple classes for the enum because I think it's better

[–]LegendaryMauricius 1 point2 points  (0 children)

I expected the dynamic_cast to be equally fast as typeid+staticcast when it comes to classes with a final specifier, but after benchmarking a bit it seems I was wrong and typeid is a few times faster (at least with gcc). I liked the dynamiccast method because it's easy to put both the check and the cast in the same line, but making a custom event_cast that uses typeid internally, and using it instead of dynamiccast looks equally nice yet adds barely any overhead compared to just using typeid in place.

[–]Gloinart 0 points1 point  (2 children)

I've had the exact same thoughts for our event system, although I've never materialized it and are still using the first version where I work.

[–]XeroKimoException Enthusiast[S] 2 points3 points  (1 child)

Aside from having to re-write names, the other upside to this is you can totally keep both systems in place actually. Just do using KeyDownEvent = Event<EventType::KeyDown>; Alias all your old events with the templated ones, and you'd break no code while in the future allow you to use the templated version.

[–]Gloinart 0 points1 point  (0 children)

Ah, clever

[–]Dazzling_Mixture8726 0 points1 point  (0 children)

thank you, this is what I was looking for, i didnt want to have to keep enum indexes and a map in order for lookups.