Refactor split_at/split_to #663
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
The shallow clone call just above always results in the kind being shared so we don't need to assert it here.
we already assert this at the top of the method
We know several things here: 1. self.len <= self.cap, always 2. at <= self.len, asserted at the top of this method 3. after calling shallow_clone, other.cap == self.cap Therefore, at <= self.len <= other.cap.
This method never moves the cursor backward, only advances it forwards. I think reflecting that in the name makes things a bit more clear. I also added explicit safety comments to make it clear why each usage is sound.
braddunbar
force-pushed
the
split-at-to-refactor
branch
from
c70d6d2 to
442f85d
Compare
carllerche
approved these changes
Feb 6, 2024
| other.len = at; | ||
| // SAFETY: We've checked that `at` <= `self.len()` and we know that `self.len()` <= | ||
| // `self.capacity()`. | ||
| self.advance_unchecked(at); |
There was a problem hiding this comment.
Very minor nitpick, but this is unsafe code so I think it might be worth applying. In split_off, you call advance_unchecked first then update cap and len. You reverse it here. Would you mind keeping them the same?
There was a problem hiding this comment.
Yeah, thanks for pointing it out! Made things a little more consistent in b971743. Let me know if that isn't what you meant.
Merged
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.
I did a little refactoring of
split_atandsplit_toto remove some extraneous checks and add some clarity around safety guarantees.Each commit is fairly self-contained, but here are the highlights with some rationale:
lena little more concisely in ea9dd60.split_atandsplit_toare doing redundant bounds checking and comparison that I removed in 851b59b and 8381ab7.set_starttoadvance_uncheckedto better reflect it's usage in c70d6d2. I also added some explicit comments about the safety guarantees of that method and an explanation of why each usage is sound.I'm pretty new to this repo, so if I've missed some rules or norms please let me know. I'll be happy to change things up!