Skip to content
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

fix treat_as_floating_point_v<_DurationType> #2638

Merged
merged 3 commits into from
Apr 16, 2022

Conversation

fsb4000
Copy link
Contributor

@fsb4000 fsb4000 commented Apr 5, 2022

Fixes #2636

@fsb4000 fsb4000 requested a review from a team as a code owner April 5, 2022 02:56
@StephanTLavavej StephanTLavavej added the bug Something isn't working label Apr 5, 2022
@StephanTLavavej StephanTLavavej self-assigned this Apr 6, 2022
stl/inc/chrono Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej removed their assignment Apr 14, 2022
@StephanTLavavej
Copy link
Member

Thanks! (I especially appreciate the added test coverage.)

To avoid the need to suppress compiler warnings, I went ahead and pushed what looks like a major rearrangement, but is actually a few simple transformations:

  • Use De Morgan's law to flip the condition.
  • After an early return, we don't need else.
  • Separate if (compile_time_condition && runtime_condition) into nested if constexpr and if.
  • Reduce the scope of using _CastedType to where it's needed.

I believe that the resulting code reads naturally: "At compile-time, if we're not treating the rep as floating-point, then we're treating it as integral (which might lose precision). We'll need _CastedType. Perform a runtime check for precision loss - if duration_cast returns a non-equal value, we've lost precision and must immediately return false. Otherwise, we're good to go for updating _Result."

For the treat-as-floating-point case, there is an extremely minor throughput advantage to if constexpr (don't need to instantiate duration_cast<_CastedType>) although I was originally motivated by the compiler warning suppression.

Hope this is okay - it's a bit larger than the changes I usually push without asking.

@fsb4000
Copy link
Contributor Author

fsb4000 commented Apr 14, 2022

I was originally motivated by the compiler warning suppression

Good.
Yes, Visual C++ compiler said me the same: "C4127: conditional expression is constant. Consider using 'if constexpr' statement instead"
But I looked at the code and I found that we have 3 other places with:

#pragma warning(push)
#pragma warning(disable : 4127) // conditional expression is constant

So this is why I did "pragma push"...
I should have listened to the compiler :)

@StephanTLavavej
Copy link
Member

Ah, thanks - I had forgotten we still had some of those. I'll go through and clean up (in a separate PR) as many of the 4127 suppressions as possible/convenient (most of them date back to when we couldn't use if constexpr unconditionally). The ones in <span> are probably more trouble to change than they're worth.

@StephanTLavavej StephanTLavavej self-assigned this Apr 15, 2022
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit 9f0e33b into microsoft:main Apr 16, 2022
@StephanTLavavej
Copy link
Member

Thanks again for fixing this bug and testing the affected parsing scenario! 😻 🐞 🎉

@fsb4000 fsb4000 deleted the fix2636 branch April 16, 2022 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

chrono: Questionable usage of treat_as_floating_point_v<_DurationType>
3 participants