Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

partialeq_ne_impl lint documentation incorrect #2893

Open
gnzlbg opened this issue Jul 3, 2018 · 2 comments
Open

partialeq_ne_impl lint documentation incorrect #2893

gnzlbg opened this issue Jul 3, 2018 · 2 comments
Labels
S-needs-discussion Status: Needs further discussion before merging or work can be started

Comments

@gnzlbg
Copy link
Contributor

gnzlbg commented Jul 3, 2018

The documentation for the partialeq_ne_impl states:

Therefore, there should never be any need to re-implement it.

but this is incorrect since re-implementing PartialEq::ne makes often a lot of sense for performance purposes where it can be implemented more efficiently than just !eq.

@oli-obk
Copy link
Contributor

oli-obk commented Jul 3, 2018

Do you have an example where it is more efficient? I have had this discussion several times (and I've been on your side at the beginning). I wasn't able to actually find any example unless you already know how your runtime data is statistically laid out, at which point I believe you should not be using the comparison operators, but extra custom code.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Jul 3, 2018

This holds for every type for which ne can be implemented more efficiently than !eq(a, b).

One example that comes to mind for which the impact is not minimal is all the mask types in std (std::simd::m{...}) , which implement ne as a.vertical_ne(b).any(). The default implementation !eq(a,b) which after inlining is !a.vertical_eq(b).all() (note that all lanes are compared in both cases, so this wouldn't apply to vector, slices, etc.).

There are also many manual implementations of ne within std (for pretty much every std type), although I don't know the motivation for most of these. Looking at some of them I'd expect LLVM to optimize many cases, but providing a manual implementation saves LLVM from having to do any work there.

@oli-obk oli-obk added the S-needs-discussion Status: Needs further discussion before merging or work can be started label Jul 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-needs-discussion Status: Needs further discussion before merging or work can be started
Projects
None yet
Development

No branches or pull requests

2 participants