`float_cmp` changes #11948

Jarcho · 2023-12-10T12:43:13Z

changelog: float_cmp_const: Deprecate in favor of a config option on float_cmp.
changelog: float_cmp: Don't lint self comparisons.
changelog: float_cmp: Don't lint literal comparisons.
changelog: float_cmp: Add a config option to ignore comparisons to named constants.
changelog: float_cmp: Add a config option to ignore const-evaluatable comparisons.
changelog: float_cmp: Add a config option to ignore comparisons to the modification of an operand (e.g. x == f(x)).

rustbot · 2023-12-10T12:43:18Z

r? @giraffate

(rustbot has picked a reviewer for you, use r? to override)

bors · 2023-12-29T11:08:59Z

☔ The latest upstream changes (presumably #10283) made this pull request unmergeable. Please resolve the merge conflicts.

Jarcho · 2024-02-14T00:25:06Z

r? clippy

bors · 2024-02-19T09:34:24Z

☔ The latest upstream changes (presumably #12306) made this pull request unmergeable. Please resolve the merge conflicts.

xFrednet · 2024-04-01T10:08:00Z

Hey @Jarcho woudl you mind rebasing this PR?

Hey @GuillaumeGomez, if you have the time, could you give this PR a review?

r? xFrednet

clippy_lints/src/operators/float_cmp.rs

GuillaumeGomez · 2024-04-01T18:12:44Z

tests/ui-toml/float_cmp_named_constants/test.rs

+#![deny(clippy::float_cmp)]
+
+fn main() {
+    {


Could you move the const C outside of the main function and also move both f functions out too please? Having nested levels like this make the code reading a bit more complicated than it could be.

Nested blocks both move the code for each test closer together (as in, everything needed for each test is all in one spot). and prevents name collisions between tests.

GuillaumeGomez · 2024-04-01T18:15:07Z

clippy_lints/src/operators/float_cmp.rs

+    if matches!(op, BinOpKind::Eq | BinOpKind::Ne)
+        && let left = peel_hir_expr_while(left, peel_expr)
+        && let right = peel_hir_expr_while(right, peel_expr)
        && is_float(cx, left)


What do you think about:

Suggested change

if matches!(op, BinOpKind::Eq | BinOpKind::Ne)

&& let left = peel_hir_expr_while(left, peel_expr)

&& let right = peel_hir_expr_while(right, peel_expr)

&& is_float(cx, left)

if !matches(op, BinOpKind::Eq | BinOpKind::Ne) {

return;

}

let left = peel_hir_expr_while(left, peel_expr);

let right = peel_hir_expr_while(right, peel_expr);

if is_float(cx, left)

?

Let chains with infallible bindings are used all over Clippy.

I agree with @Jarcho, this is the normal way and readable, IMO.

@Jarcho, if you rebase I can review this PR sometime this week. If you say it's ready

GuillaumeGomez · 2024-04-01T18:16:19Z

tests/ui-toml/float_cmp_constant_comparisons/test.rs

@@ -15,4 +15,9 @@ fn main() {
        }
        let _ = f(1.0) == f(2.0);
    }
+    {


No need to put it into a nested block. However, a comment explaining that this is checking that the lint is not emitted for literal comparisons would be nice. 👍

clippy_lints/src/operators/float_cmp.rs

GuillaumeGomez · 2024-04-01T18:18:25Z

clippy_lints/src/operators/float_cmp.rs

@@ -105,6 +113,78 @@ fn is_allowed(val: &Constant<'_>) -> bool {
    }
 }

+// This is a best effort guess and may have false positives and negatives.
+fn is_pure_expr<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> bool {


What are you trying to check here exactly? What is a "pure expression"?

No side effects.

Then please add a comment explaining that on the function.

Functional purity isn't a weird concept. It's also mentioned casually in comments a few times in other places.

GuillaumeGomez · 2024-04-01T18:18:55Z

tests/ui-toml/float_cmp_change_detection/test.rs

+
+#![deny(clippy::float_cmp)]
+
+fn main() {


I think you should add checks with closures as well.

GuillaumeGomez · 2024-04-01T18:20:18Z

clippy_lints/src/operators/float_cmp.rs

@@ -64,7 +69,7 @@ pub(crate) fn check<'tcx>(

        if let Some(name) = get_item_name(cx, expr) {
            let name = name.as_str();
-            if name == "eq" || name == "ne" || name == "is_nan" || name.starts_with("eq_") || name.ends_with("_eq") {
+            if name == "eq" || name == "ne" || name.starts_with("eq_") || name.ends_with("_eq") {


The name.starts_with("eq_") || name.ends_with("_eq") part is worrying me. Is it supposed to be for PartialEq methods? If so, please check the full name and that it is from PartialEq (same for eq and ne).

That isn't even added in this PR.

Indeed but the worry stays. Is there already a test for it by any chance? If so just resolve this conversation please.

bors · 2024-06-27T17:02:37Z

☔ The latest upstream changes (presumably #12999) made this pull request unmergeable. Please resolve the merge conflicts.

…constants to `float_cmp`

… value.

rustbot assigned giraffate Dec 10, 2023

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Dec 10, 2023

m-rph mentioned this pull request Dec 29, 2023

[epic] False Positive tracking / triaging / categorization #12046

Open

24 tasks

Jarcho force-pushed the float_cmp branch 4 times, most recently from 147961e to e0a1fc6 Compare February 14, 2024 00:24

rustbot assigned matthiaskrgr and unassigned giraffate Feb 14, 2024

Jarcho force-pushed the float_cmp branch from e0a1fc6 to 4a454d4 Compare February 14, 2024 00:52

rustbot assigned xFrednet and unassigned matthiaskrgr Apr 1, 2024

GuillaumeGomez reviewed Apr 1, 2024

View reviewed changes

clippy_lints/src/operators/float_cmp.rs Show resolved Hide resolved

GuillaumeGomez reviewed Apr 1, 2024

View reviewed changes

clippy_lints/src/operators/float_cmp.rs Show resolved Hide resolved

GuillaumeGomez reviewed Apr 1, 2024

View reviewed changes

clippy_lints/src/operators/float_cmp.rs Outdated Show resolved Hide resolved

GuillaumeGomez reviewed Apr 1, 2024

View reviewed changes

Jarcho force-pushed the float_cmp branch 2 times, most recently from f768816 to b6623ac Compare June 7, 2024 21:47

Check more constant forms in float_cmp

f20a076

Jarcho added 3 commits July 11, 2024 16:31

Add config options to ignore constant comparisons and comparisons to …

16acb2a

…constants to `float_cmp`

Deprecate float_cmp_const

4d23fb6

float_cmp: Don't lint literal to literal comparisons

0c2041f

Jarcho force-pushed the float_cmp branch from b6623ac to aed5751 Compare July 11, 2024 20:34

Jarcho added 2 commits July 11, 2024 17:19

float_cmp: Ignore comparisons to detect if an operation changes the…

effa7c5

… value.

float_cmp: Don't lint self comparisons.

e99d2ed

Jarcho force-pushed the float_cmp branch from aed5751 to e99d2ed Compare July 11, 2024 21:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`float_cmp` changes #11948

`float_cmp` changes #11948

Jarcho commented Dec 10, 2023

rustbot commented Dec 10, 2023

bors commented Dec 29, 2023

Jarcho commented Feb 14, 2024

bors commented Feb 19, 2024

xFrednet commented Apr 1, 2024

GuillaumeGomez Apr 1, 2024

Jarcho Apr 12, 2024

GuillaumeGomez Apr 1, 2024

Jarcho Jul 9, 2024

xFrednet Jul 9, 2024

xFrednet Jul 9, 2024

GuillaumeGomez Apr 1, 2024

GuillaumeGomez Apr 1, 2024

Jarcho Apr 12, 2024

GuillaumeGomez Jun 28, 2024

Jarcho Jul 9, 2024

GuillaumeGomez Apr 1, 2024

GuillaumeGomez Apr 1, 2024

Jarcho Apr 12, 2024

GuillaumeGomez Jun 28, 2024

bors commented Jun 27, 2024


		#![deny(clippy::float_cmp)]

		fn main() {

float_cmp changes #11948

Are you sure you want to change the base?

float_cmp changes #11948

Conversation

Jarcho commented Dec 10, 2023

rustbot commented Dec 10, 2023

bors commented Dec 29, 2023

Jarcho commented Feb 14, 2024

bors commented Feb 19, 2024

xFrednet commented Apr 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Jun 27, 2024

`float_cmp` changes #11948

`float_cmp` changes #11948