[Clang 10] Fix -Wimplicit-int-float-conversion compilation warnings in WTF This is a sub-task of Bug 204834. Clang 10 reports a compilation warning: > wtf/MathExtras.h(401,43): warning: implicit conversion from 'unsigned long long' to 'double' changes value from 18446744073709551615 to 18446744073709551616 [-Wimplicit-int-float-conversion] > double fmodValue = fmod(trunc(d), std::numeric_limits<unsigned long long>::max() + 1.0);
Created attachment 395357 [details] Patch
Comment on attachment 395357 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395357&action=review Looks good. review- because I think we need to deal with the float/double thing > Source/WTF/wtf/MathExtras.h:405 > +constexpr float powerOfTwo(unsigned e) > +{ > + float p = 1; > + for (unsigned i = 0; i < e; i++) > + p *= 2; > + return p; > +} > + > +template<typename T> constexpr float maxPlusOne = powerOfTwo(countOfNumberBits<T>); I think we need this separately for float and double. Just computing the float one and then converting to double won’t give the correct answer. > Source/WTF/wtf/MathExtras.h:414 > + double fmodValue = fmod(trunc(d), maxPlusOne<unsigned long long>); This seems like the right idea, but we would want it for double, not float. > Source/WTF/wtf/MediaTime.cpp:128 > + if (doubleTime >= maxPlusOne<int64_t>) This needs to be double instead of float. > Source/WTF/wtf/MediaTime.cpp:135 > + while (std::round(doubleTime * timeScale) >= maxPlusOne<int64_t>) I don’t understand why std::round is necessary here.
Comment on attachment 395357 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395357&action=review > Source/WTF/wtf/MathExtras.h:395 > +template<typename T> constexpr unsigned countOfNumberBits = countOfBits<T> - std::numeric_limits<T>::is_signed; Could use std::is_signed_v<T> instead of std::numeric_limits<T>::is_signed I think this should probably be named countOfMagnitudeBits.
Comment on attachment 395357 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395357&action=review >> Source/WTF/wtf/MathExtras.h:405 >> +template<typename T> constexpr float maxPlusOne = powerOfTwo(countOfNumberBits<T>); > > I think we need this separately for float and double. Just computing the float one and then converting to double won’t give the correct answer. Hmm, maybe I am wrong about that. I guess powers of two are particularly well represented in floating point and perhaps converting the float one to double works perfectly. Best way to prove that would be testing, but I am guessing I was wrong.
Created attachment 395411 [details] test-float-power-of-two.cpp float has 8bit exponent, which can represent -126〜127. (-127 and 128 are use for NaN and Inf) I created a test program to convert float powerOfTwo to double, and decomposeDouble of it. > (...) > 126 126 1.000000 > 127 127 1.000000 > 128 1024 1.000000 > 129 1024 1.000000 > (..) powerOfTwo(128) overflows. It is safe to calculate maxPlusOne for 64bit integer type. Should I add ASSERT or static_assert? powerOfTwo needs template programing for using static_assert.
Comment on attachment 395357 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395357&action=review >> Source/WTF/wtf/MediaTime.cpp:135 >> + while (std::round(doubleTime * timeScale) >= maxPlusOne<int64_t>) > > I don’t understand why std::round is necessary here. Here is the code without the first std::round: > while (doubleTime * timeScale >= maxPlusOne<int64_t>) > timeScale /= 2; > return MediaTime(static_cast<int64_t>(std::round(doubleTime * timeScale)), timeScale, Valid); If doubleTime * timeScale is pow(2, 63) - 0.5, the condition would be false. However, std::round(doubleTime * timeScale) will round up, and will be pow(2, 63). static_cast<int64_t>(pow(2, 63)) is undefined. However, ordinary 'double' type can't keep the precision of pow(2, 63) - 0.5. I think it's safe to omit the first std::round .
Created attachment 395439 [details] Patch
Comment on attachment 395439 [details] Patch Looks good to me like this
Committed r259537: <https://trac.webkit.org/changeset/259537> All reviewed patches have been landed. Closing bug and clearing flags on attachment 395439 [details].
<rdar://problem/61302881>