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

Generate less code when deriving #31972

Closed
bluss opened this issue Feb 29, 2016 · 9 comments
Closed

Generate less code when deriving #31972

bluss opened this issue Feb 29, 2016 · 9 comments
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. I-compiletime Issue: Problems and improvements with respect to compile times.

Comments

@bluss
Copy link
Member

bluss commented Feb 29, 2016

Here are some ideas for generating less code when deriving. All of these break down with generic fields, since then we have to fall back to the current general scheme.

PartialEq

For C-like enums, only produce eq and not ne. The default ne calls eq and is always enough. Edit: implemented in #31977.

PartialOrd

For C-like enums only produce partial_cmp and not the four other methods lt, le, gt, ge. This needs an experiment to see if only partial_cmp is good enough. Maybe another of the single methods can be enough, too.

PartialOrd, Ord together

When there are no generic parameters, simply call .cmp() in .partial_cmp().

cc @durka

@durka
Copy link
Contributor

durka commented Feb 29, 2016

Seem like good ideas in general. Why do you assume that omitting ne is good but omitting gt might not be?

Do we have a derive-heavy crate somewhere that'd be good for compile-time and runtime benchmarks?

@bluss
Copy link
Member Author

bluss commented Feb 29, 2016

ne just uses !eq, but gt uses a slightly more complicated expression based on the return value of partial_cmp.

@ollie27
Copy link
Member

ollie27 commented Mar 3, 2016

Does it make sense to derive an implementation of ne for any types? It's impossible to implement it in a way that's faster than !eq.

@durka
Copy link
Contributor

durka commented Mar 3, 2016

@ollie27 if we don't call ne() for the inner types, then it's an observable behavior change.

@ollie27
Copy link
Member

ollie27 commented Mar 3, 2016

While it's technically observable we have a rule "!(a == b) if and only if a != b" in PartialEq so any PartialEq implementation for which it is a behaviour change is broken anyway.

@durka
Copy link
Contributor

durka commented Mar 3, 2016

While I agree this was not a convincing argument over at #31414 so I recommend we keep this PR small :)

@bluss
Copy link
Member Author

bluss commented Mar 3, 2016

Why do we even have two methods then?

@ollie27 I agree with your observation. And isn't it so that any fast path to either proving equality or nonequality (whichever is easier), is applicable in both .eq and .ne?

@ollie27
Copy link
Member

ollie27 commented Mar 3, 2016

Yeah, if there's a fast path for one then the fast path for the other is just negating the output. I don't think there's ever a reason to override ne() and I think it only exists so the operator != has something to map to.

@steveklabnik steveklabnik added the A-codegen Area: Code generation label Mar 11, 2016
@Mark-Simulacrum Mark-Simulacrum added C-enhancement Category: An issue proposing an enhancement or a PR with one. I-compiletime Issue: Problems and improvements with respect to compile times. and removed A-codegen Area: Code generation labels Jul 24, 2017
@Mark-Simulacrum
Copy link
Member

performance testing for leftover cases seems to indicate this is mostly a wash on real-world code so closing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. I-compiletime Issue: Problems and improvements with respect to compile times.
Projects
None yet
Development

No branches or pull requests

5 participants