Changed locale retrieval way to a fancy one #2594
Conversation
toughengineer
force-pushed
the
get_locale
branch
from
d513ccc
to
74855cb
Compare
include/fmt/chrono.h
Outdated
|
|
||
| private: | ||
| union { | ||
| monostate dummy{}; |
There was a problem hiding this comment.
monostate is C++17 only
I'm sorry!!
This is fmt::monostate, not std::monostate.
There was a problem hiding this comment.
It's monostate from core.h.
There was a problem hiding this comment.
{} seems unnecessary, please remove.
There was a problem hiding this comment.
Strictly speaking this is unnecessary.
But without monostate default initialized, this is kinda scary because you have a generally unintialized member, and invariants become kinda hard to follow.
All this is OK for experts, but I imagine that this is harder to follow for non-experts.
monostate has negligible overhead, but helps to figure out what's happening more easily.
That's why I added monostate member.
There was a problem hiding this comment.
monostate is fine but I don't think you need to initialize it explicitly at least because it has no state =).
There was a problem hiding this comment.
If you never initialize monostate dummy, than there is no point in having it at all.
IMHO, initializing monostate dummy by default gives some understanding and peace of mind to the person trying to understand the code.
Initializing it by default says that the lifetime of the dummy object is started, and you need to explicitly do something to change it.
In this case when locale is initialized using placement new, it explicitly says that the lifetime of dummy ends and another object starts to occupy the storage simultaneously starting its lifetime.
If there was no monostate dummy it would work perfectly correctly, but then a person trying to understand the code must keep in mind that the union is generally uninitialized.
toughengineer
force-pushed
the
get_locale
branch
from
74855cb
to
e2727a2
Compare
|
Yep, I screwed up, but now it seems to be fixed. |
|
No, my idea is perf slower :( |
|
LGTM but please rebase. |
toughengineer
force-pushed
the
get_locale
branch
from
e2727a2
to
dc34f6e
Compare
|
Merged, thanks! |
This is a quick and dirty one, but I like it much, much better than #2592.
I hope I haven't screwed up anything.
And this one is not slowed down compared to the ugly one with lamdas and whatnot.