Bug 209955 – [Clang 10] Fix -Wimplicit-int-float-conversion compilation warnings in WTF

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Fujii Hironori]

[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);

[Fujii Hironori]

Created attachment 395357 [details]
Patch

[Darin Adler]

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.

[Darin Adler]

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.

[Darin Adler]

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.

[Fujii Hironori]

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.

[Fujii Hironori]

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 .

[Fujii Hironori]

Created attachment 395439 [details]
Patch

[Darin Adler]

Comment on attachment 395439 [details]
Patch

Looks good to me like this

[EWS]

Committed r259537: <https://trac.webkit.org/changeset/259537>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 395439 [details].

[Radar WebKit Bug Importer]

<rdar://problem/61302881>