Summary: | [Clang 10] Fix -Wimplicit-int-float-conversion compilation warnings in WTF | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Fujii Hironori <Hironori.Fujii> | ||||||||
Component: | Web Template Framework | Assignee: | Fujii Hironori <Hironori.Fujii> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | benjamin, cdumez, cmarcelo, darin, eric.carlson, ews-watchlist, glenn, jer.noble, philipj, sergio, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 204834 | ||||||||||
Attachments: |
|
Description
Fujii Hironori
2020-04-03 01:00:27 PDT
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]. |