Implement the empty event state with std::monostate

[ChrisThrasher]

Description

The idiomatic way of representing a default empty state of a std::variant is to use std::monostate also from the <variant> header. This is a change that users should not notice since users already should not be using sf::Event::Empty. This saves us a bit of code that we don't need to write.

[coveralls]

Pull Request Test Coverage Report for Build 9020111123

Details

• 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 51.897%

---

Totals
Change from base Build 9020114451:	0.0%
Covered Lines:	10474
Relevant Lines:	19002

---

💛 - Coveralls

[vittorioromeo]

users already should not be using sf::Event::Empty

It is public though -- why do we have an empty state in the variant? Maybe some APIs should return a std::optional<Event> instead of relying on Event::Empty?

[ChrisThrasher]

It is public though

It has public visibility but it's never something users were expected to use or know about. It only existed as an implementation detail.

Maybe some APIs should return a std::optional instead of relying on Event::Empty?

I considered this and wrote the patch to see what it would look like but decided it was too annoying to use pointer semantics and scrapped the idea.

[vittorioromeo]

It is public though

It has public visibility but it's never something users were expected to use or know about. It only existed as an implementation detail.

This suggests an area for improvement to me. It is an encapsulation violation that it's public, part of the API, but it shouldn't be used at all.

Maybe some APIs should return a std::optional instead of relying on Event::Empty?

I considered this and wrote the patch to see what it would look like but decided it was too annoying to use pointer semantics and scrapped the idea.

As far as I can see, the only reason why Empty even exists is popEvent. Can't we just change that function to return an optional event? I don't think we need pointer semantics at all, but I am likely missing something. I am not home right now so my browsing/editing capabilities are a bit limited.

[vittorioromeo]

Created #2992 as a possible alternative design, I think it is superior.

[eXpl0it3r]

I don't think this is the correct move.

We talk about "empty state" in the documentation, yet don't define what that means exactly and if someone takes a look at the code, will see that they could use std::monostate, as such exposing the implementation detail of std::variant on the public API.

The fact that "users should not notice" or "never something users were expected to use or know about" doesn't really matter, since we are exposing it publicly, regardless of shoulds and shouldn'ts. And who's to say, that they shouldn't be "allowed" to use the empty state instead of the bool operator?

[ChrisThrasher]

We talk about "empty state" in the documentation, yet don't define what that means exactly

We do define that. We define it as operator bool evaluating to false.

if someone takes a look at the code, will see that they could use std::monostate, as such exposing the implementation detail of std::variant on the public API.

Nothing more about the underlying std::variant is exposed by this change. std::monostate is an empty type just like sf::Event::Empty is an empty type.

And who's to say, that they shouldn't be "allowed" to use the empty state instead of the bool operator?

We make the rules. I'm not sure why we have to pretend we're powerless to dictate correct usage.

Anyways, it's not worth fighting for this PR so I'll give you this one. I don't see how we benefit from rewriting something already in the standard library.