[webkit-reviews] review denied: [Bug 209955] [Clang 10] Fix -Wimplicit-int-float-conversion compilation warnings in WTF : [Attachment 395357] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Fujii Hironori
<Hironori.Fujii at sony.com>'s request for review:
Bug 209955: [Clang 10] Fix -Wimplicit-int-float-conversion compilation warnings
in WTF
https://bugs.webkit.org/show_bug.cgi?id=209955

Attachment 395357: Patch

https://bugs.webkit.org/attachment.cgi?id=395357&action=review




--- Comment #2 from Darin Adler <darin at apple.com> ---
Comment on attachment 395357
  --> https://bugs.webkit.org/attachment.cgi?id=395357
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.