add manual_sign_check lint

[justDeeevin]

changelog: [manual_sign_check]: add lint

Lints on things like f < 0 and 0 <= ffor floats, recommending the use of f*::is_positive/f*::is_negative instead. Added to pedantic group.

[rustbot]

r? @samueltardieu

rustbot has assigned @samueltardieu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[github-actions]

Lintcheck changes for a235c82

Lint	Added	Removed	Changed
clippy::manual_sign_check	15	0	0

This comment will be updated if you push new changes

[samueltardieu]

Looking at the lintcheck result shows some inconsistencies:

345 |             Value::F32(value) => Value::F32(if value < 0. { -value } else { value }),
    |                                                ^^^^^^^^^^ help: consider using `is_sign_negative` for clarity and performance: `value.is_sign_negative()`

Not equivalent: if value is -0.0, the initial test will evaluate to false while .is_sign_negative() will evaluate to true

166 |         if n >= 0.0 {
    |            ^^^^^^^^ help: consider using `is_sign_negative` for clarity and performance: `!n.is_sign_negative()`

Why not n.is_sign_positive()?

361 |         if fragments[j - 1].penalty_width() > 0.0 {
    |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `is_sign_positive` for clarity and performance: `fragments[j - 1].penalty_width().is_sign_positive()`

Not equivalent: if the expression evaluates to 0.0, the comparaison will evaluate to false while is_sign_positive() will evaluate to true

You should add more tests, including with edge cases. I suggest that you use a const fn that evaluates expressions such as:

const fn check_properties(x: f32) {
    assert!((x >= 0.0) == (x.is_positive());
    assert!((x <= -0.0) == (x.is_negative());
   // … include other tests here, with conditions reversed, etc.
}

fn main() {
    const {
        check_properties(0.0);
        check_properties(-0.0);
        check_properties(1.0);
        check_properties(-1.0);
        check_properties(f32::INFINITY);
        check_properties(f32::NEG_INFINITY);
        check_properties(f32::NAN);
        check_properties(f32::MIN);
        check_properties(f32::MAX);
    }
}

to make sure that expressions are really equivalent, both in the test file and the fixed file, as it will not compile if expressions are not equivalent.

View changes since this review

[justDeeevin]

Not equivalent: if value is -0.0, the initial test will evaluate to false while .is_sign_negative() will evaluate to true

This is a caveat that is noted in the documentation both for the lint and the method itself. I'm not sure there's any way around this, and working with negative zero seems like an edge case anyways?

[justDeeevin]

Also thank you for the feedback—this is my first time contributing to this repo and to rust in general and I appreciate the helpfulness :)

[justDeeevin]

Why not n.is_sign_positive()?

In what is admittedly somewhat of an inconsistency, this is kind of bringing -0.0 into consideration—-0.0==0.0 so to check for >=0.0 it's slightly more accurate to check that sign isn't negative

[samueltardieu]

Not equivalent: if value is -0.0, the initial test will evaluate to false while .is_sign_negative() will evaluate to true

This is a caveat that is noted in the documentation both for the lint and the method itself. I'm not sure there's any way around this, and working with negative zero seems like an edge case anyways?

In this case, it cannot be in the pedantic category. You should probably start with nursery, then we could think about putting it into the restriction category.