New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement the empty event state with std::monostate
#2991
Conversation
Pull Request Test Coverage Report for Build 9020111123Details
💛 - Coveralls |
It is public though -- why do we have an empty state in the variant? Maybe some APIs should return a |
It has public visibility but it's never something users were expected to use or know about. It only existed as an implementation detail.
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. |
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.
As far as I can see, the only reason why |
Created #2992 as a possible alternative design, I think it is superior. |
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 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? |
We do define that. We define it as
Nothing more about the underlying
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. |
Description
The idiomatic way of representing a default empty state of a
std::variant
is to usestd::monostate
also from the<variant>
header. This is a change that users should not notice since users already should not be usingsf::Event::Empty
. This saves us a bit of code that we don't need to write.