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

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect, P2)

Description

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 2

[Sylvestre Ledru [:Sylvestre]]

I am fixing more than this just one :)

Comment 3

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1577236 - clang-10: Fix -Wimplicit-int-float-conversion warnings in gfx/ r?nical

Comment 4

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1577236 - clang-10: Fix -Wimplicit-int-float-conversion warnings in dom/media r?jya

Depends on D43776

Comment 5

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1577236 - clang-10: Fix -Wimplicit-int-float-conversion warnings in dom/smil r?birtles

Depends on D43778

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1577236 - clang-10: Fix -Wimplicit-int-float-conversion warnings in dom/animation/ r?hiro

Depends on D43779

Comment 7

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1577236 - clang-10: Fix -Wimplicit-int-float-conversion warnings in dom/base/ r?ehsan

Depends on D43780

Comment 8

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1577236 - clang-10: Fix -Wimplicit-int-float-conversion warnings in dom/vr/ r?kip

Depends on D43781

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1577236 - clang-10: Fix -Wimplicit-int-float-conversion warnings in /js/ r?jandem

Depends on D43782

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1577236 - clang-10: Fix -Wimplicit-int-float-conversion warnings in /layout/ r?jwatt

Depends on D43783

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1577236 - clang-10: Fix -Wimplicit-int-float-conversion warnings in various dirs r?glandium

Depends on D43784

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1577236 - clang-10: Fix a -Wimplicit-int-float-conversion warning in chromium r?bobowen

Depends on D43785

Comment 13

[Jeff Walden [:Waldo]]

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.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/776b650e4d08
clang-10: Fix -Wimplicit-int-float-conversion warnings in gfx/ r=nical

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9dcc8e550559
clang-10: Fix -Wimplicit-int-float-conversion warnings in dom/animation/ r=hiro

Comment 16

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/776b650e4d08
https://hg.mozilla.org/mozilla-central/rev/9dcc8e550559

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d4b4d46545b3
clang-10: Fix -Wimplicit-int-float-conversion warnings in dom/smil r=birtles

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/80d4cec02dc9
clang-10: Fix -Wimplicit-int-float-conversion warnings in dom/base/ r=Ehsan

Comment 19

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/d4b4d46545b3
https://hg.mozilla.org/mozilla-central/rev/80d4cec02dc9

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fcc8383a1c4f
clang-10: Fix -Wimplicit-int-float-conversion warnings in /js/ r=jandem

Comment 21

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/fcc8383a1c4f

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6eb3bfbbad08
clang-10: Fix -Wimplicit-int-float-conversion warnings in various dirs r=glandium

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/127fb176c023
clang-10: Fix -Wimplicit-int-float-conversion warnings in dom/media r=jya

Comment 24

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/6eb3bfbbad08
https://hg.mozilla.org/mozilla-central/rev/127fb176c023

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/da9a6db4b59b
clang-10: Fix -Wimplicit-int-float-conversion warnings in dom/vr/ r=kip

Comment 26

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/da9a6db4b59b

Comment 27

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d4b4d46545b3
https://hg.mozilla.org/releases/mozilla-beta/rev/80d4cec02dc9
https://hg.mozilla.org/releases/mozilla-beta/rev/fcc8383a1c4f
https://hg.mozilla.org/releases/mozilla-beta/rev/6eb3bfbbad08
https://hg.mozilla.org/releases/mozilla-beta/rev/127fb176c023
https://hg.mozilla.org/releases/mozilla-beta/rev/da9a6db4b59b

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7876583b2407
clang-10: Fix -Wimplicit-int-float-conversion warnings in /layout/ r=jwatt

Comment 29

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e24768876059
Backed out changeset 7876583b2407 for causing failures in calc-rounding-001.html CLOSED TREE

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d443a2006a04
clang-10: Fix -Wimplicit-int-float-conversion warnings in /layout/ r=jwatt

Comment 31

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/d443a2006a04

Comment 32

[(Away)]

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

Comment 33

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1577236 - clang-10: Fix -Wimplicit-int-float-conversion warnings in gfx/ r?nical

The change has been reverted involuntary by bug 1574745

Comment 34

[Sylvestre Ledru [:Sylvestre]]

Thanks, I pushed:
https://phabricator.services.mozilla.com/D47734

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/749b087bd8e0
clang-10: Fix -Wimplicit-int-float-conversion warnings in gfx/ r=nical

Comment 36

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/749b087bd8e0

Comment 37

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/749b087bd8e0

Comment 38

[(Away)]

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

Comment 39

[(Away)]

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.

Comment 40

[(Away)]

Attachment: clang-10: Fix -Wimplicit-int-float-conversion warnings in TimeStamp

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4b979a591152
clang-10: Fix -Wimplicit-int-float-conversion warnings in TimeStamp r=froydnj

Comment 42

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/4b979a591152

Comment 43

[Sylvestre Ledru [:Sylvestre]]

Published a new version:
https://phabricator.services.mozilla.com/D43786

Comment 44

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/03a3ed508dd8
clang-10: Fix a -Wimplicit-int-float-conversion warning in chromium r=bobowen

Comment 45

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/03a3ed508dd8

Comment 46

[(Away)]

Attachment: clang-10: Fix a -Wimplicit-int-float-conversion warning in ExtendInputEffectD2D1

Comment 47

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/96f6073b246a
clang-10: Fix a -Wimplicit-int-float-conversion warning in ExtendInputEffectD2D1 r=jrmuizel

Comment 48

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/96f6073b246a

Comment 49

[Sylvestre Ledru [:Sylvestre]]

I think we are now good.