Remove pow(floating, int) overloads by Berrysoft · Pull Request #903 · microsoft/STL (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation14 Commits11 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Fixes #890

As MSVC supports at least C++14, I simply removed the pow(floating, int) overloads, because they should be removed after C++11.

I also found that the template one could be reduced to _GENERIC_MATH2 macro, so I did it.

Tests are also added.

All CLA requirements met.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this bug! The product changes look correct to me; I have comments for the test coverage.

Seems that it has a internal name VSO-553723, so I use that as the name of test.

Could you please Tag the github issue of PR instead. MSFT internal Tickets are not publicly accessible.

@miscco Could you please Tag the github issue of PR instead.

Name changed.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed 3 small changes (and a merge with master):

New tests need to be added to test.lst.
That revealed that the test was failing; it should expect pow(float, float) to be float. I double-checked all cases; thanks for the comprehensive coverage.
Purely stylistic, I thought it was more consistent to initialize all of long long, int, and short with literal 2. (Widening int to long long is value-preserving, even before considering that 2 is a compile-time constant. The latter is why the compiler accepts initializing a short without emitting a truncation warning.)

I thought about the previous special-casing for pow(floating, 2); this shouldn't cause correctness issues (I expect the UCRT pow to do the right thing). There might be a performance issue, since previously we would have skipped the function call. However, additional branches aren't free, so I think that making this change is fine. If we receive performance bug reports, we can investigate them later. (Also, if users really really want to square numbers quickly, they should probably be writing x * x directly.)

Now that the tests are passing, I'll begin porting this to the Microsoft-internal repo in preparation for merging!

@CaseyCarter and @BillyONeal reminded me that we now have a // COMPILE-ONLY convention for tests that don't execute any code at runtime, so I pushed one more change to follow this convention.

Thanks for fixing this highly visible bug, simplifying the codebase, improving the test coverage, and congratulations on your first microsoft/STL commit! 🎉 😸

ahanamuk pushed a commit to ahanamuk/STL that referenced this pull request

Jul 1, 2020

Fixes microsoft#890.

Co-authored-by: Stephan T. Lavavej stl@microsoft.com

N-Dekker added a commit to biovault/biovault_bfloat16 that referenced this pull request

Nov 11, 2020

It appears that with Visual C++ 2019 Version 16.8.0, expressions like std::pow(2.0f, int{ exponent }) return a double, while with a previous Visual C++ version, it returned a float. This new behavior appears introduced with "Remove pow(floating, int) overloads", pull request microsoft/STL#903 It caused the following warnings:

biovault_bfloat16_test.cpp(225,37): error C2220: the following warning is treated as an error biovault_bfloat16_test.cpp(225,37): warning C4244: 'argument': conversion from 'double' to 'const float', possible loss of data biovault_bfloat16_test.cpp(239,37): warning C4244: 'argument': conversion from 'double' to 'const float', possible loss of data

This commit fixes the warnings by calling C++11 std::powf instead.

N-Dekker added a commit to biovault/biovault_bfloat16 that referenced this pull request

Nov 11, 2020

biovault_bfloat16_test.cpp(225,37): error C2220: the following warning is treated as an error biovault_bfloat16_test.cpp(225,37): warning C4244: 'argument': conversion from 'double' to 'const float', possible loss of data biovault_bfloat16_test.cpp(239,37): warning C4244: 'argument': conversion from 'double' to 'const float', possible loss of data

This commit fixes the warnings by making sure that the second argument of std::pow(2.0f, exponent) calls is a float, instead of an integer.

N-Dekker added a commit to biovault/biovault_bfloat16 that referenced this pull request

Nov 12, 2020

biovault_bfloat16_test.cpp(225,37): error C2220: the following warning is treated as an error biovault_bfloat16_test.cpp(225,37): warning C4244: 'argument': conversion from 'double' to 'const float', possible loss of data biovault_bfloat16_test.cpp(239,37): warning C4244: 'argument': conversion from 'double' to 'const float', possible loss of data

This commit fixes the warnings by making sure that the second argument of std::pow(2.0f, exponent) calls is a float, instead of an integer.

This was referenced

Oct 6, 2025

Labels

bug

Something isn't working