Remove pow(floating, int) overloads #903
Conversation
|
|
|
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. |
Name changed. |
There was a problem hiding this comment.
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 befloat. 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, andshortwith literal2. (Wideninginttolong longis value-preserving, even before considering that2is a compile-time constant. The latter is why the compiler accepts initializing ashortwithout 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 |
|
Thanks for fixing this highly visible bug, simplifying the codebase, improving the test coverage, and congratulations on your first microsoft/STL commit! 🎉 😸 |
Fixes microsoft#890. Co-authored-by: Stephan T. Lavavej <stl@microsoft.com>
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.
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 making sure that the second argument of `std::pow(2.0f, exponent)` calls is a `float`, instead of an integer.
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 making sure that the second argument of `std::pow(2.0f, exponent)` calls is a `float`, instead of an integer.
5 participants
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_MATH2macro, so I did it.Tests are also added.