- Makes wording more clear and re-structures some
sections that can be overwhelming for some not
already in the know.
- Adds examples of how *not* to implement Ord,
inspired by various anti-patterns found in real
world code.
There is a Self: PartialOrd bound in Ord::clamp, but it is already
required by the trait itself. Likely a left-over from the const trait
deletion in 76dbe29104.
Reported-by: @noeensarguet
Document overrides of `clone_from()` in core/std
As mentioned in https://github.com/rust-lang/rust/pull/96979#discussion_r1379502413
Specifically, when an override doesn't just forward to an inner type, document the behavior and that it's preferred over simply assigning a clone of source. Also, change instances where the second parameter is "other" to "source".
I reused some of the wording over and over for similar impls, but I'm not sure that the wording is actually *good*. Would appreciate feedback about that.
Also, now some of these seem to provide pretty specific guarantees about behavior (e.g. will reuse the exact same allocation iff the len is the same), but I was basing it off of the docs for [`Box::clone_from`](https://doc.rust-lang.org/1.75.0/std/boxed/struct.Box.html#method.clone_from-1) - I'm not sure if providing those strong guarantees is actually good or not.
This reverts commit 049a917535.
The resolution to <https://github.com/rust-lang/rust/issues/123282> is
that the `f16`/`f128` regression in the beta compiler was fixable
without a revert, so the commit adding `#[cfg(not(bootstrap))]` is no
longer useful (added in
<https://github.com/rust-lang/rust/pull/123390>).
Revert this commit because not having these basic impls bootstrap-gated
simplifies everything else that uses them.
Add `Ord::cmp` for primitives as a `BinOp` in MIR
Update: most of this OP was written months ago. See https://github.com/rust-lang/rust/pull/118310#issuecomment-2016940014 below for where we got to recently that made it ready for review.
---
There are dozens of reasonable ways to implement `Ord::cmp` for integers using comparison, bit-ops, and branches. Those differences are irrelevant at the rust level, however, so we can make things better by adding `BinOp::Cmp` at the MIR level:
1. Exactly how to implement it is left up to the backends, so LLVM can use whatever pattern its optimizer best recognizes and cranelift can use whichever pattern codegens the fastest.
2. By not inlining those details for every use of `cmp`, we drastically reduce the amount of MIR generated for `derive`d `PartialOrd`, while also making it more amenable to MIR-level optimizations.
Having extremely careful `if` ordering to μoptimize resource usage on broadwell (#63767) is great, but it really feels to me like libcore is the wrong place to put that logic. Similarly, using subtraction [tricks](https://graphics.stanford.edu/~seander/bithacks.html#CopyIntegerSign) (#105840) is arguably even nicer, but depends on the optimizer understanding it (https://github.com/llvm/llvm-project/issues/73417) to be practical. Or maybe [bitor is better than add](https://discourse.llvm.org/t/representing-in-ir/67369/2?u=scottmcm)? But maybe only on a future version that [has `or disjoint` support](https://discourse.llvm.org/t/rfc-add-or-disjoint-flag/75036?u=scottmcm)? And just because one of those forms happens to be good for LLVM, there's no guarantee that it'd be the same form that GCC or Cranelift would rather see -- especially given their very different optimizers. Not to mention that if LLVM gets a spaceship intrinsic -- [which it should](https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/Suboptimal.20inlining.20in.20std.20function.20.60binary_search.60/near/404250586) -- we'll need at least a rustc intrinsic to be able to call it.
As for simplifying it in Rust, we now regularly inline `{integer}::partial_cmp`, but it's quite a large amount of IR. The best way to see that is with 8811efa88b (diff-d134c32d028fbe2bf835fef2df9aca9d13332dd82284ff21ee7ebf717bfa4765R113) -- I added a new pre-codegen MIR test for a simple 3-tuple struct, and this PR change it from 36 locals and 26 basic blocks down to 24 locals and 8 basic blocks. Even better, as soon as the construct-`Some`-then-match-it-in-same-BB noise is cleaned up, this'll expose the `Cmp == 0` branches clearly in MIR, so that an InstCombine (#105808) can simplify that to just a `BinOp::Eq` and thus fix some of our generated code perf issues. (Tracking that through today's `if a < b { Less } else if a == b { Equal } else { Greater }` would be *much* harder.)
---
r? `@ghost`
But first I should check that perf is ok with this
~~...and my true nemesis, tidy.~~
This saves some debug and scope metadata in every single function that calls it.
Normally wouldn't be worth it, but with the derives there's *so* many of these.
Specifically, when an override doesn't just forward to an inner type,
document the behavior and that it's preferred over simply assigning
a clone of source. Also, change instances where the second parameter is
"other" to "source".
PartialEq, PartialOrd: update and synchronize handling of transitive chains
It was brought up in https://internals.rust-lang.org/t/total-equality-relations-as-std-eq-rhs/19232 that we currently have a gap in our `PartialEq` rules, which this PR aims to close:
> For example, with PartialEq's conditions you may have a = b = c = d ≠ a (where a and c are of type A, b and d are of type B).
The second commit fixes https://github.com/rust-lang/rust/issues/87067 by updating PartialOrd to handle the requirements the same way PartialEq does.
Explanation that fields are being used when deriving `(Partial)Ord` on enums
When deriving `std::cmp::Ord` or `std::cmp::PartialOrd` on enums, their fields are compared if the variants are equal.
This means that the last assertion in the following snipped panics.
```rust
use std::cmp::{PartialEq, Eq, PartialOrd, Ord};
#[derive(PartialEq, Eq, PartialOrd, Ord)]
enum Sizes {
Small(usize),
Big(usize),
}
fn main() {
let a = Sizes::Big(3);
let b = Sizes::Big(5);
let c = Sizes::Small(10);
assert!( c < a);
assert_eq!(a, c);
}
```
This is more often expected behavior than not, and can be easily circumvented, as discussed in [this thread](https://users.rust-lang.org/t/how-to-sort-enum-variants/52291/4).
But it is addressed nowhere in the documentation, yet.
So I stumbled across this, as I personally did not expect fields being used in `PartialOrd`.
I added the explanation to the documentation.
Add lint against ambiguous wide pointer comparisons
This PR is the resolution of https://github.com/rust-lang/rust/issues/106447 decided in https://github.com/rust-lang/rust/issues/117717 by T-lang.
## `ambiguous_wide_pointer_comparisons`
*warn-by-default*
The `ambiguous_wide_pointer_comparisons` lint checks comparison of `*const/*mut ?Sized` as the operands.
### Example
```rust
let ab = (A, B);
let a = &ab.0 as *const dyn T;
let b = &ab.1 as *const dyn T;
let _ = a == b;
```
### Explanation
The comparison includes metadata which may not be expected.
-------
This PR also drops `clippy::vtable_address_comparisons` which is superseded by this one.
~~One thing: is the current naming right? `invalid` seems a bit too much.~~
Fixes https://github.com/rust-lang/rust/issues/117717
Some clarifications regarding used (mathematical) terminology:
* Avoid using the terms "total equality" and "partial equality" in favor
of "equivalence relation" and "partial equivalence relation", which
are well-defined and unambiguous.
* Clarify that `Ordering` is an ordering between two values (and not an
order in the mathematical sense).
* Avoid saying that `PartialEq` and `Eq` are "equality comparisons"
because the terminology "equality comparison" could be misleading:
it's possible to implement `PartialEq` and `Eq` for other (partial)
equivalence relations, in particular for relations where `a == b` for
some `a` and `b` even when `a` and `b` are not the same value.
* Added a section "Strict and non-strict partial orders" to document
that the `<=` and `>=` operators do not correspond to non-strict
partial orders.
* Corrected section "Corollaries" in documenation of Ord in regard to
`<` only describing a strict total order in cases where `==` conforms
to mathematical equality.
Made documentation easier to understand:
* Explicitly state at the beginning of `PartialEq`'s documentation
comment that implementing the trait will provide the `==` and `!=`
operators.
* Added an easier to understand rule when to implement `Eq` in addition
to `PartialEq`: "if it’s guaranteed that `PartialEq::eq(a, a)` always
returns `true`."
* Explicitly mention in documentation of `Eq` that the properties
"symmetric" and "transitive" are already required by `PartialEq`.
fix a comment about assert_receiver_is_total_eq
"a type implements #[deriving]" doesn't make any sense, so I assume they meant "implement `Eq`"? Also the attribute is called `derive`.
Add `minmax{,_by,_by_key}` functions to `core::cmp`
This PR adds the following functions:
```rust
// mod core::cmp
#![unstable(feature = "cmp_minmax")]
pub fn minmax<T>(v1: T, v2: T) -> [T; 2]
where
T: Ord;
pub fn minmax_by<T, F>(v1: T, v2: T, compare: F) -> [T; 2]
where
F: FnOnce(&T, &T) -> Ordering;
pub fn minmax_by_key<T, F, K>(v1: T, v2: T, mut f: F) -> [T; 2]
where
F: FnMut(&T) -> K,
K: Ord;
```
(they are also `const` under `#[feature(const_cmp)]`, I've omitted `const` stuff for simplicity/readability)
----
Semantically these functions are equivalent to `{ let mut arr = [v1, v2]; arr.sort(); arr }`, but since they operate on 2 elements only, they are implemented as a single comparison.
Even though that's basically a sort, I think "sort 2 elements" operation is useful on it's own in many cases. Namely, it's a common pattern when you have 2 things, and need to know which one is smaller/bigger to operate on them differently.
I've wanted such functions countless times, most recently in #109402, so I thought I'd propose them.
----
r? libs-api
clarify that unsafe code must not rely on our safe traits
This adds a disclaimer to PartialEq, Eq, PartialOrd, Ord, Hash, Deref, DerefMut.
We already have a similar disclaimer in ExactSizeIterator (worded a bit differently):
```
/// Note that this trait is a safe trait and as such does *not* and *cannot*
/// guarantee that the returned length is correct. This means that `unsafe`
/// code **must not** rely on the correctness of [`Iterator::size_hint`]. The
/// unstable and unsafe [`TrustedLen`](super::marker::TrustedLen) trait gives
/// this additional guarantee.
```
If there are any other traits that should carry such a disclaimer, please let me know.
Fixes https://github.com/rust-lang/rust/issues/73682
