Bug 209955 - [Clang 10] Fix -Wimplicit-int-float-conversion compilation warnings in WTF
Summary: [Clang 10] Fix -Wimplicit-int-float-conversion compilation warnings in WTF
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Fujii Hironori
URL:
Keywords: InRadar
Depends on:
Blocks: 204834
  Show dependency treegraph
 
Reported: 2020-04-03 01:00 PDT by Fujii Hironori
Modified: 2020-04-04 14:43 PDT (History)
11 users (show)

See Also:


Attachments
Patch (4.85 KB, patch)
2020-04-03 01:16 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
test-float-power-of-two.cpp (1.62 KB, text/plain)
2020-04-03 14:36 PDT, Fujii Hironori
no flags Details
Patch (4.94 KB, patch)
2020-04-03 23:36 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fujii Hironori 2020-04-03 01:00:27 PDT
[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);
Comment 1 Fujii Hironori 2020-04-03 01:16:25 PDT
Created attachment 395357 [details]
Patch
Comment 2 Darin Adler 2020-04-03 09:09:56 PDT
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 3 Darin Adler 2020-04-03 09:12:20 PDT
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 4 Darin Adler 2020-04-03 09:40:35 PDT
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.
Comment 5 Fujii Hironori 2020-04-03 14:36:30 PDT
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 6 Fujii Hironori 2020-04-03 23:24:09 PDT
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 .
Comment 7 Fujii Hironori 2020-04-03 23:36:13 PDT
Created attachment 395439 [details]
Patch
Comment 8 Darin Adler 2020-04-04 14:19:55 PDT
Comment on attachment 395439 [details]
Patch

Looks good to me like this
Comment 9 EWS 2020-04-04 14:42:29 PDT
Committed r259537: <https://trac.webkit.org/changeset/259537>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 395439 [details].
Comment 10 Radar WebKit Bug Importer 2020-04-04 14:43:15 PDT
<rdar://problem/61302881>