reduce repetition and document overrides of the default impl of PartialEq::ne

[hkBst]

Removes manual impls of PartialEq::ne which are equivalent to the default.

• Cleans up documentation of PartialEq::ne and documents overrides of the default impl.
• Uses macros to remove duplication.

[rustbot]

r? @tgross35

rustbot has assigned @tgross35.
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

[taiki-e]

equivalent to the default

Note that implementations for reference types are not strictly the same as the default because they are forwarding of the underlying implementation. In such cases, if the compiler cannot optimize the inversion of the result from the underlying implementation (e.g., inline assembly or FFI is used), then ne may generate an extra 1-2 instructions (xor with constant 1 or set constant 1 to the register and xor with it).

[hkBst]

@taiki-e that is a good point, thanks!

[workingjubilee]

I don't think we should add macros to appease the thing that is linting in an effort to make the code more readable.

Adding macros is rather the opposite of making the code more readable.

[hkBst]

@workingjubilee macros like these are used all over the library for repeating impls just like here... Do you feel the same way about those, or can you explain why you think these are any different?

[workingjubilee]

If you look you will find that I have pushed people to reduce the usage of macros in other PRs, yes.

[hkBst]

Very well, I was just trying to clarify your point. Macros come with the disadvantage of more complicated code, but one advantage of using macros like this is that it is easier to see which code is identically duplicated and which parts contain differences. Opinions will vary on in which cases the upside outweighs the downside.

My latest change introduced 3 macros:

• The first repeats something for all combinations of two references each of which can be mut or not, so 4 times total. To me this seems justified.
• The second repeats a larger impl block 2 times, which also seems justified. It also raises the question of the 2 missing impls, to which I don't know the answer.
• The last repeats a simple impl for ordinary and mutable refs, so 2 times. It is also very simple, so also seems justified to me, but I don't feel very strongly about this one.

[tgross35]

I don't have any strong preferences here

r? workingjubilee

[rustbot]

workingjubilee is currently at their maximum review capacity.
They may take a while to respond.

[bors]

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

[fee1-dead]

Primitive ne isn't equivalent to !(*self == *other). This difference goes straight to MIR and codegen:

rust/compiler/rustc_codegen_ssa/src/mir/rvalue.rs

Lines 822 to 833 in 0fb279b

	mir::BinOp::Ne
	| mir::BinOp::Lt
	| mir::BinOp::Gt
	| mir::BinOp::Eq
	| mir::BinOp::Le
	| mir::BinOp::Ge => {
	if is_float {
	bx.fcmp(base::bin_op_to_fcmp_predicate(op), lhs, rhs)
	} else {
	bx.icmp(base::bin_op_to_icmp_predicate(op, is_signed), lhs, rhs)
	}
	}

Yes it can be trivially optimized away but why emit additional IR when you can emit less?

[hkBst]

Interesting! I suppose that treatment may be necessary in case these are implemented by inline assembly or via FFI.

Yes it can be trivially optimized away but why emit additional IR when you can emit less?

The documentation in this very file says that:

/// The default implementation of `ne` provides this consistency and is almost
/// always sufficient. It should not be overridden without very good reason.

Is the performance impact of emitting slightly more IR really a very good reason? I would expect it is on the edge of immeasurable.

[workingjubilee]

Is the performance impact of emitting slightly more IR really a very good reason? I would expect it is on the edge of immeasurable.

That is a very interesting question since we do in fact have some measurements! Let's at least find out if our existing ones would say anything.

[fee1-dead]

My reservation wasn't mainly about the performance impact. I'm thinking of alternative codegen backends that read Rust MIR and generate code with instruction sets that have ne as well as eq for primitives.

Rather than telling people to rely on MIR opts or LLVM opts (when the backend may not be LLVM) I'd prefer us to do something better on the outset.

The other thing is that it just feels weird? I just don't think clippy is very applicable to this specific case.

[fee1-dead]

And like, what's the benefit here? Of course we don't have to be biased towards the status quo but what is the difference between the change and the status quo?

These are just two lines in a macro. Removing them isn't too much different from keeping them.

[hkBst]

The difference would be that the code would then do what the comment about implementing ne seems to recommend. Removing ne is just one of three ways of achieving that, and merely the one that seemed most likely to work at the time. But I'm also happy to document the "very good reason" why we want to impl ne here, or change the comment or both.

[hkBst]

I've now worked a bit on the documentation, mentioning primitive impls of == and !=. The only removed ne impl now is the one for ()...

[bors]

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

[workingjubilee]

The question of measurable performance impact if any was raised so we might as well test this much, even if it's not the be-all end-all of performance testing since our runtime benches remain lacking:

@bors try @rust-timer queue

[hkBst]

@workingjubilee I think I addressed all raised concerns. Would be great if you could take another look!

[workingjubilee]

thanks.

@bors r+ rollup

[bors]

📌 Commit 6912f15 has been approved by workingjubilee

It is now in the queue for this repository.

[Zalathar]

Failed in rollup: #149828 (comment)

@bors r-

This failure was hard to track down, because it was caused by a code change in a PR that was labelled as a docs update.

[hkBst]

This failure was hard to track down, because it was caused by a code change in a PR that was labelled as a docs update.

@Zalathar Apologies for leading you astray. In my defense, this failure seems to have been caused by ui tests that didn't exist when this change set was last tested, which included the name of an internal macro that got renamed in this change set.

[workingjubilee]

You're right, @Zalathar, the PR title and description should be accurate instead of misleading.

[bors]

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

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.