MSVC improvements and data truncation cleanup. #739
Conversation
|
Since part of the reason for the timidity about not evaluating internal::get_types at compile time is because they've been buggy, do you think there should be explicit tests validating the correctness of TYPES, or do you believe the existing tests should be good enough? |
| @@ -73,7 +73,8 @@ | |||
| # endif | |||
| #endif | |||
|
|
|||
| #if FMT_HAS_FEATURE(cxx_explicit_conversions) | |||
| #if FMT_HAS_FEATURE(cxx_explicit_conversions) || \ | |||
| FMT_MSC_VER >= 1800 | |||
There was a problem hiding this comment.
MSVC 18.0 is the minimum supported version in master, so the check is not needed.
There was a problem hiding this comment.
Since MSVC doesn't support __has_feature, surely there needs to be some check? FMT_MSC_VER > 0 seems like a silly test.
There was a problem hiding this comment.
My bad, not sure what I was thinking about =).
include/fmt/core.h
Outdated
| #if FMT_USE_CONSTEXPR | ||
| static constexpr uint64_t TYPES = | ||
| IS_PACKED ? internal::get_types<Context, Args...>() | ||
| : -static_cast<int64_t>(NUM_ARGS); |
There was a problem hiding this comment.
Please factor out the initializer into a separate function and reuse here and below to reduce copy-paste.
What do you mean by get_types being buggy? They are not (always) |
|
Sorry, I don't mean get_types is buggy. I meant something along the lines of "are the existing tests enough of a sanity check to be 'certain' that get_types doesn't come across any bugs in MSVC's fragile constexpr implementation?" I didn't want to make things worse for MSVC users for something that may not have any tangible benefits. As you noted, the computation is optimized away, but it leaves behind a mutable global. New mutable globals trigger some internal reviews at my company. It's easy enough to filter them out in this instance, but not needing to filter them out is even easier. |
Got it. I think existing tests should be enough.
Sure. I'm OK with the change, but please address the remaining inline comment above. |
MSVC is timid about evaluating constexpr functions unless it has to, so the "TYPES" variables end up in read-write memory even though the optimizer removes the initializer. Making TYPES constexpr causes MSVC to try harder to initialize these variables at compile time, which also ends up completely removing the (named) variable from the final compiled binary. Fixed a data truncation warning being reported in ostream-test.
|
Addressed. I wanted to wait for your guidance on the explicit conversion thing since I figured I was missing something. |
|
Merged, thanks! |
MSVC is timid about evaluating constexpr functions unless it has to, so the "TYPES" variables end up in read-write memory even though the optimizer removes the initializer. Making TYPES constexpr causes MSVC to try harder to initialize these variables at compile time, which also ends up completely removing the (named) variable from the final compiled binary.
Fixed a data truncation warning being reported in ostream-test.