Remix.run Logo
▲ nananana9 4 hours ago

We've been doing this for 50 years, you don't need a class hierarchy for your event struct.

  enum EventType {
      EVENT_NONE,
      EVENT_TEMPERATURE_READING,
      EVENT_SMOKE_DETECTED,
      EVENT_CARBON_MONOXIDE_DETECTED,
  };
  struct Event {
      EventType type;
      union {
          TemperatureDeadingData temperature_reading;
          SmokeDetectedData smoke_detected;
          CarbonMonoxideDetected carbon_monoxide_detected
      } data;
  };

  std::vector<Event> events;
▲cataphract 2 hours ago | parent | next [-]

Yes, the article covers this (mentions the std::variant). This makes it a closed set though. Other differences is that your Event here is as big your biggest event type, and it doesn't go through normal inheritance mechanisms. With polymorphic you could just have an abstract Event and implement your subclasses normally.

A way to see it is some sort of type-erasure over destruction and copy-construction (plus another corner case for mismatched allocators).

The constructor of polymorphics captures the type of the subclass during its constructor (they didn't make the same mistake as with shared_ptr of giving it a pointer constructor, like the UB in case the destructor is not virtual in Base *b; shared_ptr<Base> sp{b}). With this, it can instantiate a template (a control block) where the subclass is known.

▲josefx 3 hours ago | parent | prev | next [-]

> We've been doing this for 50 years

Code that turns into an unmaintainable mess the moment even one of the types involved contains dynamically allocated data? Yes, seen that thousands of times.

The article creates a type that has typesafety, resource management and basic copy/assignment operations backed in.

▲OskarS 3 hours ago | parent | prev | next [-]

Yes, though in modern C++, don't do this, use std::variant instead. It has better type-safety (nothing stops you from writing to one member of the union but reading from another, causing UB) and handles lifetimes much better. Your version leaks, because struct Event wont destruct any of the nested types inside the union, and in the example, they have heap-allocated data (std::strings and the like). In order to that properly with a tagged union, you need to define the destructor of Event yourself (which is not done in your example) and call the correct destructor manually of the active union member.

Tagged unions (especially with resource-managing types) is hard to implement correctly in C++ (and not trivial in C either). Don't do it, use std::variant.

▲quietbritishjim 4 hours ago | parent | prev | next [-]

That doesn't work if you are writing a library where users of the library are able to implement their own event types.

Even for a self-contained code base, it's a pain compared to a type that just works with any current or future derived class.

▲nananana9 4 hours ago | parent | next [-]

It does.

  enum EventType {
     ...,
     EVENT_USER_START = 0xF0000000,
  };
  struct UserEventData { uint8 placeholder[64]; }
  struct Event {
     EventType type;
     union {
         UserEventData user;
     };
  }
That aside, have you ever felt the need to implement your own Win32 event, or SDL event, or X11 event, or epoll_event, or a custom DOM event or whatever?

If you're writing a good library, give the user the data they need, and let them handle it however they wish, don't force them int your framework.

▲quietbritishjim 2 hours ago | parent | next [-]

That doesn't work if your library needs to handle these events somehow, not just transparently pass them around.

Typically with your pattern, you'd use a switch statement on the event type, but what do you do if it's in the user-defined space (type>=EVENT_USER_START)? You could add mandatory function pointers for relevant handlers in a header prefix, but now you're just reimplementing virtual methods. As is common in C, you end up making your own object-oriented language, only with more effort and not as good.

▲marckerbiquet 3 hours ago | parent | prev [-]

It does not work if two independent libraries depend on your library and create the same user EventType.

▲gpderetta 3 hours ago | parent | prev [-]

https://en.wikipedia.org/wiki/Expression_problem

▲adev_ 4 hours ago | parent | prev | next [-]

What you wrote there is just an unsafe way to do std::variant. This has nothing to do with what the blog post is about.

The blog is about Open polymorphism. Where the list of compatible Object can be extended dynamically (Which is handy for some type of error report for instance).

You can do that since forever in C++ (Since the first support for inheritance), the only thing std::polymorphism brings is the ability to do it without dynamic allocations, emplacement new or lifetime problems.

▲nananana9 4 hours ago | parent [-]

> This has nothing to do with what the blog post is about.

It's an alternative way to solve the problem presented, which trivially allows you to make a std::vector<Event> (the whole point the article is building up to).

> the only thing std::polymorphism brings is the ability to do it without dynamic allocations, emplacement new or lifetime problems.

Which the version I showed has none of.

▲adev_ 4 hours ago | parent | next [-]

> it's an alternative way to solve the problem presented

No it's not and you apparently did not understand the concept behind it (or more likely, do not want to understand the concept behind it).

Your solution is not an implementation of Open Polymorphism, it is just a poor man implementation of std::variant.

> Which the version I showed has none of.

Because it is not the same thing like explain by my previous comment and in around 5 other comments around mine soon I guess.

But you are entirely free to grunt around and continue to imply that C++ committee people are implementing something useless (Which they are not, this is useful).

▲nananana9 4 hours ago | parent [-]

My point is that you don't need open polymorphism for the example given, and if you give me another example, I can show you why you don't need it there either -- because you do in fact never need it. It's an XY problem over the set X.

If a programmer is reaching for a library, they want it to solve a concrete task. Nobody wants to subclass your Event type, if I have to do it, you've failed at your job.

▲adev_ 2 hours ago | parent [-]

> Nobody wants to subclass your Event type, if I have to do it, you've failed at your job.

Because it's well known that subclassing is used only for Event type and never EVER anything else requires polymorphism #irony.

The only part of this discussion where you are right is that closed polymorphism (std:: variant) is better than open polymorphism when it can be applied.

And even there, your proposed implementation is utter garbage that leaks memory and that I would refuse straight away in any PR.

▲OskarS 3 hours ago | parent | prev [-]

> Which the version I showed has none of.

No, it has something much worse, it has a memory leak. The nested types inside the union allocate memory, and they don't get destructed when Event gets destructed. Don't do this in modern C++, it's a terrible terrible idea, these kinds of constructs are very hard to get correct manually. Just use std::variant.

▲fooker 4 hours ago | parent | prev | next [-]

When this piece of code exists in a library, it makes it pretty much impossible to extend by the user of the library.

▲Panzerschrek 4 hours ago | parent | prev [-]

Typical C approach. In C++ this should be done with std::variant instead. It has the exact same binary layout, but is type-safe (compared to union usage).