reduce repetition with macros 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

[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.

[hkBst]

@rustbot ready

[hkBst]

I've improved the headline, so it can't be mistaken for a docs only commit so easily...

[hkBst]

@workingjubilee Anything else you need to push this over the line (again)?

[workingjubilee]

It seems I have been finding half-reasons to put off looking at this despite doing other reviews. Partially because I find the conclusion regrettable as I allowed too much work to occur before getting to it. Partially because I was hoping my conclusion was just some passing mood. And, er, it seems it hasn't passed.

Over the course of this I had been trying to persuade myself that I should be reasonable, and that a significantly cut-down form of the original PR could have some merit here. But in this case that changed when this finally bounced on CI another time, I felt like I had to insist on an update to the PR description, and then saw the expansion of the subject to include two things that feel unrelated. All together, that managed to eat the last of my belief that this is a good PR to land, even in a reduced form. What of this is improvement versus change for change's sake, I can no longer tell, nor do I think we will benefit by sending it in and letting future revisions iterate on top of it.

I must close this. My apologies for letting this drag out.

[hkBst]

@workingjubilee Thanks for letting me know. I suppose I shall try to separate this change into parts and submit those separately in order to salvage the value that I think is in here. I hope you'll not mind my doing so.

[workingjubilee]

The code changes introduced here do not seem to have sufficient merit, to me, to warrant their inclusion, even when separated. Mere avoidance of repetition is not a virtue if it also obfuscates things.

[workingjubilee]

@hkBst In short: I do mind. If I can only convince you to submit smaller and smaller, more atomized PRs, and you then reviewer-shop until you find someone who agrees, then I do not have any actual discretion here. My rejection meaning nothing means my approval means nothing. And apparently I should not ever again explain my thought process around declining a PR lest it be interpreted as mere suggestion.