-
Notifications
You must be signed in to change notification settings - Fork 4
add vector slice #1
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
Open
elcritch
wants to merge
9
commits into
paranim:master
Choose a base branch
from
elcritch:add-vec-slice
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+104
−5
Open
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
c9ef1ef
add vector slice
elcritch 2c76a10
fix merge
elcritch 1252fa5
add for set
elcritch f0668de
remove in
elcritch e6c083d
merge
elcritch e0eb29e
add vector delete
elcritch 9d76d75
add vector delete
elcritch e5901b8
fixup case for adding to slice
elcritch a319c32
fixup case for adding to slice
elcritch File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is correct. At this point we are reaching a leaf that already exists and updating its value, so the size should not change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, you inspired me to look into the size stuff and I realized I am actually incrementing too often. Specifically, if you use
setLento increase the size of a vec, and then set a value in one of the new slots, it is incrementing the size when it should not:7fc8d3a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setLenisn't used when adding a new item, at least forVec[T]. In my test case when you added an item to the seq, the length wasn't changing like it should.To handle both a
setLenand adding new items, you'll might want a capacity and a length. That's howseqworks I think.Though, perhaps you could update add to only increment if
index == size? Then I think my change would be correct for cases when you're adding at the end, and fix the issue you pointed out.Or you could re-work add to always use
setLenwhenindex >= size.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know; I didn't mean to imply that it was. It was a separate bug that I happened to notice yesterday.
That is exactly what I did. I linked to the commit in my last comment.