Explicitly deref in float partial_eq

[Warrenren]

The current code will not results bug, but it difficult to understand. These code result to call &f32::partial_cmp(), and the performance will be lower than the changed code. I'm not sure why the current code don't use (*self) (*other), if you have some idea, please let me know.

[rust-highfive]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @kennytm (or someone else) soon.

Please see the contribution instructions for more information.

[eggyal]

There isn't an implementation of PartialOrd for &f32, so the values are implicitly dereferenced.

[Warrenren]

When I debug, the code go to the following implementation. And this for all &T which implement PartialOrd. So f32::partial_cmp call &f32::le() and &f32::ge(). To avoid going here, should change "self" to "*self" and "other" to "*other"

impl<A: ?Sized, B: ?Sized> PartialOrd<&B> for &A
where
A: PartialOrd,
{
#[inline]
fn partial_cmp(&self, other: &&B) -> Option {
PartialOrd::partial_cmp(*self, *other)
}
#[inline]
fn lt(&self, other: &&B) -> bool {
PartialOrd::lt(*self, *other)
}
#[inline]
fn le(&self, other: &&B) -> bool {
PartialOrd::le(*self, *other)
}
#[inline]
fn gt(&self, other: &&B) -> bool {
PartialOrd::gt(*self, *other)
}
#[inline]
fn ge(&self, other: &&B) -> bool {
PartialOrd::ge(*self, *other)
}
}

[kadiwa4]

There is such an implementation (docs), but it shouldn't have an impact on performance. Do you have an example of a performance improvement?

[eggyal]

Ah yes, I'd forgotten there was a blanket impl for references. But it's all inlined and optimised away (at least in release mode) so I'm not sure it's really a big problem? That said, I can't see any issue with making this change.

[Warrenren]

There is such an implementation (docs), but it shouldn't have an impact on performance. Do you have an example of a performance improvement?

I have made a comment, I have some problem to describe the issue and the comment correct the description. If using "self", there will call fn which could be avoid.

[Noratrieb]

In my opinion, this does not make the code clearer but only adds more noise. This should not have any perf impact, as the compiler usually optimizes these calls away. Godbolt confirms this: https://godbolt.org/z/5TazWeees

[Warrenren]

Ah yes, I'd forgotten there was a blanket impl for references. But it's all inlined and optimised away (at least in release mode) so I'm not sure it's really a big problem? That said, I can't see any issue with making this change.

Actually, using "*self" will make it easy to understand. I read it for a lot time and at last debug to know what happened. Though there is not issue, I still suggest to change it to make it easy to understand

[Warrenren]

For other fn, the "*self " is used, why this different

In my opinion, this does not make the code clearer but only adds more noise. This should not have any perf impact, as the compiler usually optimizes these calls away. Godbolt confirms this: https://godbolt.org/z/5TazWeees

For other fn, the "*self " is used, why this different.

[kadiwa4]

With -Copt-level=0, Godbolt registers an improvement. But I suggest removing the parantheses, because this also works:
*self <= *other

[Warrenren]

With -Copt-level=0, Godbolt registers an improvement. But I suggest removing the parantheses, because this also works: *self <= *other

With -Copt-level=0, Godbolt registers an improvement. But I suggest removing the parantheses, because this also works: *self <= *other

I write it because other fn use parantheses, so make them same .

[Dylan-DPC]

@bors r+ rollup

[bors]

📌 Commit 9e1e476 has been approved by Dylan-DPC

[klensy]

Current PR title is actually bad :-(

[Dylan-DPC]

suggestions are welcome :P

[Noratrieb]

"Explicitly deref in float partial_eq"

[klensy]

suggestions are welcome :P

I'm not fully understand reasons for this PR from description and later discussion, maybe T-libs member will comment?

[compiler-errors]

@Dylan-DPC why did this get r+'ed? It's arguable if it's an improvement in readability, and nobody from @rust-lang/libs had the chance to review it. I know it was a small PR, but what's the rush to merge it?