Open Bug 1577236 Opened 3 months ago Updated 12 hours ago

clang-10: Fix -Wimplicit-int-float-conversion warnings

Categories

(Firefox Build System :: Source Code Analysis, defect)

defect
Not set

Tracking

(firefox70 fixed)

REOPENED
mozilla70
Tracking Status
firefox70 --- fixed

People

(Reporter: Sylvestre, Assigned: Sylvestre, NeedInfo)

References

(Blocks 1 open bug)

Details

(Keywords: leave-open)

Attachments

(12 files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review

A Clang warning has been improved in https://reviews.llvm.org/D64666 and we have a bunch of occurrences in the code base

/var/lib/jenkins/workspace/firefox-clang-lld-last/obj-x86_64-pc-linux-gnu/dist/include/mozilla/FastBernoulliTrial.h:368:21: error: implicit conversion from 'unsigned long' to 'double' changes value from 18446744073709551615 to 18446744073709551616 [-Werror,-Wimplicit-int-float-conversion]
    if (skipCount < SIZE_MAX)
                  ~ ^~~~~~~~
/usr/include/stdint.h:261:22: note: expanded from macro 'SIZE_MAX'
#  define SIZE_MAX              (18446744073709551615UL)
                                 ^~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1577051

I am fixing more than this just one :)

Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---

Coincidentally I discovered this warning when I happened to need to rebuild my tip clang yesterday. I took care of one of the MFBT warning in bug 1577051 and one of the JS warnings in bug 1577065. We're hashing out a proper fix for the other JS warning in bug 1577066.

While merely making the conversions explicit is enough to silence the warning, it's not always the best, most ideal fix IMO. Inexact int-to-float conversions have only implementation-defined behavior, which is a bit more obfuscatory than writing code that doesn't depend on such. And in likely an awful lot of these cases, what is being tested is overflow conditions (because FOO_MAX is what's being cast). It often is going to be better to convert flt <= FOO_MAX to flt < float(FOO_MAX + 1 carefully calculated to not immediately overflow), which is what the JS changes ended up doing. Reviewers probably should consider similar sorts of changes to just spraying explicit conversions everywhere.

Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/776b650e4d08
clang-10: Fix -Wimplicit-int-float-conversion warnings in gfx/ r=nical
Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9dcc8e550559
clang-10: Fix -Wimplicit-int-float-conversion warnings in dom/animation/ r=hiro
Status: REOPENED → RESOLVED
Closed: 3 months ago3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
Attachment #9088760 - Attachment description: Bug 1577236 - clang-10: Fix -Wimplicit-int-float-conversion warnings in dom/siml r?birtles → Bug 1577236 - clang-10: Fix -Wimplicit-int-float-conversion warnings in dom/smil r?birtles
Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d4b4d46545b3
clang-10: Fix -Wimplicit-int-float-conversion warnings in dom/smil r=birtles
Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/80d4cec02dc9
clang-10: Fix -Wimplicit-int-float-conversion warnings in dom/base/ r=Ehsan
Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fcc8383a1c4f
clang-10: Fix -Wimplicit-int-float-conversion warnings in /js/ r=jandem
Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6eb3bfbbad08
clang-10: Fix -Wimplicit-int-float-conversion warnings in various dirs r=glandium
Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/127fb176c023
clang-10: Fix -Wimplicit-int-float-conversion warnings in dom/media r=jya
Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/da9a6db4b59b
clang-10: Fix -Wimplicit-int-float-conversion warnings in dom/vr/ r=kip
Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7876583b2407
clang-10: Fix -Wimplicit-int-float-conversion warnings in /layout/ r=jwatt
Backout by malexandru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e24768876059
Backed out changeset 7876583b2407 for causing failures in calc-rounding-001.html CLOSED TREE
Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d443a2006a04
clang-10: Fix -Wimplicit-int-float-conversion warnings in /layout/ r=jwatt

Looks like a change landed shortly after your gfx/ patch that stomped on some of your fixes: https://searchfox.org/mozilla-central/rev/23f836a71cfe961373c8bd0d0219ec60a64b3c8f/gfx/layers/opengl/CompositorOGL.cpp#946

Flags: needinfo?(sledru)
Flags: needinfo?(sledru)
Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/749b087bd8e0
clang-10: Fix -Wimplicit-int-float-conversion warnings in gfx/ r=nical

Sylvestre, are you still working on this? Looks like only one patch remains.

Flags: needinfo?(sledru)

Here is another one that only comes up on Windows:

[task 2019-11-20T19:15:08.157Z] z:/task_1574273573/src/mozglue/misc/TimeStamp_windows.cpp(385,16): error: implicit conversion from 'long long' to 'double' changes value from 9223372036854775807 to 9223372036854775808 [-Werror,-Wimplicit-int-float-conversion]
[task 2019-11-20T19:15:08.157Z]   if (result > INT64_MAX) {
[task 2019-11-20T19:15:08.157Z]              ~ ^~~~~~~~~
[task 2019-11-20T19:15:08.157Z] z:\task_1574273573\vs2017_15.8.4\VC\include\stdint.h(55,26): note: expanded from macro 'INT64_MAX'
[task 2019-11-20T19:15:08.157Z] #define INT64_MAX        9223372036854775807i64
[task 2019-11-20T19:15:08.157Z]                          ^~~~~~~~~~~~~~~~~~~~~~

Edit: I see the same code in TimeStamp_darwin.cpp too.

You need to log in before you can comment on or make changes to this bug.