1483761 - Enable some more clang warnings: -Wc++2a-compat, -Wfloat-(overflow|zero)-conversion, -Wtautological-overlap-compare

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, enhancement, P3)

Description

[Chris Peterson [:cpeterson]]

-Wc++2a-compat
-Wfloat-overflow-conversion
-Wfloat-zero-conversion
-Wtautological-overlap-compare

https://clang.llvm.org/docs/DiagnosticsReference.html#wc-2a-compat
https://clang.llvm.org/docs/DiagnosticsReference.html#wfloat-overflow-conversion
https://clang.llvm.org/docs/DiagnosticsReference.html#wfloat-zero-conversion
https://clang.llvm.org/docs/DiagnosticsReference.html#wtautological-overlap-compare

Comment 1

[Chris Peterson [:cpeterson]]

Green Try builds:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b8ff99f1f9ecce650d341f5307c44bc992a182ff

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1638b1cb9b1098c365b9254bea3377ada7f5643

Comment 2

[Chris Peterson [:cpeterson]]

Attachment: Bug 1483761 - Enable clang's -Wfloat-(overflow|zero)-conversion warnings. r=glandium

-Wfloat-overflow-conversion detects when a constant floating point value is converted to an integer type and will overflow the target type.

https://clang.llvm.org/docs/DiagnosticsReference.html#wfloat-overflow-conversion

-Wfloat-zero-conversion detects when a non-zero floating point value is converted to a zero integer value.

https://clang.llvm.org/docs/DiagnosticsReference.html#wfloat-zero-conversion

There are currently no -Wfloat-overlap-conversion warnings in mozilla-central. There is one -Wfloat-zero-conversion warning in a webrtc test. It doesn't block enabling this check because the webrtc tests are not compiled with warnings-as-errors.

media/webrtc/trunk/webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc:255:54 [-Wfloat-zero-conversion] implicit conversion from 'const float' to 'int' changes non-zero value from 0.045000002 to 0

We can't enable all -Wfloat-conversion warnings (for any implicit conversion of a floating-point number into an integer) because there are currently over 1400 warnings. I spot checked a few of these -Wfloat-conversion warnings. I didn't find any obvious bugs, but there is some suspicious code, such as implicit conversions of floats to bools.

Comment 3

[Chris Peterson [:cpeterson]]

Attachment: Bug 1483761 - Enable clang's -Wtautological-overlap-compare warning. r=glandium

-Wtautological-overlap-compare is an opt-in warning added in clang 3.5. It warns about overlapping comparisons that are always true or false, such as:

if (x > 4 || x < 10) {} // warning! always true
int b = x < 2 && x > 5; // warning! always false
return x > 4 || x < 10; // warning! always true

https://clang.llvm.org/docs/DiagnosticsReference.html#wtautological-overlap-compare

There are currently no -Wtautological-overlap-compare warnings in mozilla-central.

Depends on D3476

Comment 4

[Chris Peterson [:cpeterson]]

Attachment: Bug 1483761 - Enable clang's -Wc++2a-compat warnings. r=glandium

Warn about C++ constructs whose meaning change in C++2a.

https://clang.llvm.org/docs/DiagnosticsReference.html#wc-2a-compat

So far the only -Wc++2a-compat check that I know of is for valid pre-C++2a code that inadvertently parses as C++2a's new <=> "spaceship" comparison operator. `f<&A::operator<=>();` is an example of a warning reported for a real project on GitHub. That code can be rewritten as `f< &operator<= >();`.

There are currently no -Wc++2a-compat warnings in mozilla-central.

Depends on D3477

Comment 5

[Chris Peterson [:cpeterson]]

Attachment: Bug 1483761 - Enable clang's -Wshadow-field-in-constructor warnings. r=glandium

This opt-in warning catches bugs where a constructor modifies a constructor parameter that shadows member variable name. The code probably intended to change the member variable value, not the paramter. There are currently no -Wshadow-field-in-constructor warnings in mozilla-central.

https://clang.llvm.org/docs/DiagnosticsReference.html#wshadow-field-in-constructor-modified

Depends on D3478

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 9001511 [details]
Bug 1483761 - Enable clang's -Wfloat-(overflow|zero)-conversion warnings. r=glandium

Mike Hommey [:glandium] has approved the revision.

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 9001512 [details]
Bug 1483761 - Enable clang's -Wtautological-overlap-compare warning. r=glandium

Mike Hommey [:glandium] has approved the revision.

Comment 8

[Mike Hommey [:glandium]]

Comment on attachment 9001513 [details]
Bug 1483761 - Enable clang's -Wc++2a-compat warnings. r=glandium

Mike Hommey [:glandium] has approved the revision.

Comment 9

[Mike Hommey [:glandium]]

Comment on attachment 9002573 [details]
Bug 1483761 - Enable clang's -Wshadow-field-in-constructor warnings. r=glandium

Mike Hommey [:glandium] has approved the revision.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9a890fba236
Enable clang's -Wc++2a-compat warnings. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/36f6bab3d669
Enable clang's -Wfloat-(overflow|zero)-conversion warnings. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/f41a14a1b4de
Enable clang's -Wshadow-field-in-constructor warnings. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d30bac7078b
Enable clang's -Wtautological-overlap-compare warning. r=glandium

Comment 11

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/f9a890fba236
https://hg.mozilla.org/mozilla-central/rev/36f6bab3d669
https://hg.mozilla.org/mozilla-central/rev/f41a14a1b4de
https://hg.mozilla.org/mozilla-central/rev/6d30bac7078b

Comment 12

[Chris Peterson [:cpeterson]]

Attachment: Bug 1483761 - Enable clang's -Wfloat-(overflow|zero)-conversion warnings.

-Wfloat-overflow-conversion detects when a constant floating point value is converted to an integer type and will overflow the target type.

https://clang.llvm.org/docs/DiagnosticsReference.html#wfloat-overflow-conversion

-Wfloat-zero-conversion detects when a non-zero floating point value is converted to a zero integer value.

https://clang.llvm.org/docs/DiagnosticsReference.html#wfloat-zero-conversion

There are currently no -Wfloat-overlap-conversion warnings in mozilla-central. There is one -Wfloat-zero-conversion warning in a webrtc test. It doesn't block enabling this check because the webrtc tests are not compiled with warnings-as-errors.

media/webrtc/trunk/webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc:255:54 [-Wfloat-zero-conversion] implicit conversion from 'const float' to 'int' changes non-zero value from 0.045000002 to 0

We can't enable all -Wfloat-conversion warnings (for any implicit conversion of a floating-point number into an integer) because there are currently over 1400 warnings. I spot checked a few of these -Wfloat-conversion warnings. I didn't find any obvious bugs, but there is some suspicious code, such as implicit conversions of floats to bools.

Bug 1483761 - Enable clang's -Wtautological-overlap-compare warning. r?glandium

-Wtautological-overlap-compare is an opt-in warning added in clang 3.5. It warns about overlapping comparisons that are always true or false, such as:

if (x > 4 || x < 10) {} // warning! always true
int b = x < 2 && x > 5; // warning! always false
return x > 4 || x < 10; // warning! always true

https://clang.llvm.org/docs/DiagnosticsReference.html#wtautological-overlap-compare

There are currently no -Wtautological-overlap-compare warnings in mozilla-central.

Bug 1483761 - Enable clang's -Wc++2a-compat warnings. r?glandium

Warn about C++ constructs whose meaning change in C++2a.

https://clang.llvm.org/docs/DiagnosticsReference.html#wc-2a-compat

So far the only -Wc++2a-compat check that I know of is for valid pre-C++2a code that inadvertently parses as C++2a's new <=> "spaceship" comparison operator. f<&A::operator<=>(); is an example of a warning reported for a real project on GitHub. That code can be rewritten as f< &operator<= >();.

There are currently no -Wc++2a-compat warnings in mozilla-central.

Comment 13

[Chris Peterson [:cpeterson]]

Closing this old Phabricator revision because it was already reviewed and landed (in comment 10 above).