Improve assembly of Contiguous derive #200
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I see CI fails because |
`from_integer` and `into_integer` are usually provided by the trait's default implementation. We override this implementation because it goes through `transmute_copy`, which can lead to inefficient assembly as seen in Lokathor#175 .
Lokathor
added
semver-patch
|
Sorry this got lost for a while, but it looks great! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
from_integerandinto_integerare usually provided by the trait's default implementation. We override this implementation because it goes throughtransmute_copy, which can lead to inefficient assembly as seen in #175 .Closes #175
Implemented my suggestion from when I reported the issue. The downside of this approach is that we're duplicating code between the derive and the default implementation. The upside is the better assembly from the derived implementation. It can do better than the default implementation because it has more information.
I changed the range check to
containsbecause Clippy complained about about it and I wanted both version of the function to have the same check.Edit: Reverted because MSRV doesn't support
contains.I verified that the linked issue is fixed by this PR.