- 
                Notifications
    You must be signed in to change notification settings 
- Fork 13.9k
Replace syntax's SmallVector with AccumulateVec #37551
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
Conversation
| Also cc #37371, which has some discussion on expanding/generalizing SmallVector. | 
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.
This code would look better with @ticki's replace_with, but it's not altogether that bad without either.
| 
 LLVM gets confused by  | 
| There should probably be two types still, at least a  | 
c2ea97d    to
    2d8ee6f      
    Compare
  
    | Alright, the implementation now consists of  This currently doesn't compile, but I don't know why--from what I can tell, I didn't change the method signatures shown here. Edit: Also, as a sidenote, this error message could really be improved: I can't tell what is being moved out of borrowed content. cc @jonathandturner. | 
| That's an odd error - are you sure you didn't change a signature by accident? | 
743fb75    to
    1894f71      
    Compare
  
    | The current code segfaults with this backtrace. I don't know why this is, and would appreciate any suggestions.  | 
| Hmm, after removing the derives for   | 
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.
So I'm not sure if this causes the observed bug, but the iteration order is definitely wrong.
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.
You don't need mem::replace, you can just do ptr::read(&arr[self.count - 1].value).
Also, you can decrement first, and use self.count then.
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.
Why? You could make a forward by-value iterator if you wanted to (by having an ArrayVec and a start index), and then implement IntoIterator on ArrayVec.
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.
This is not a good idea at all, it goes in the reverse order!
86b96e4    to
    128d756      
    Compare
  
    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.
You can just self.iter() - same for the mutable version below.
cb581ec    to
    c5b951a      
    Compare
  
    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.
You can use ? instead of try! here and below.
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 was going to, but then we need to add ? to the feature list and I recalled needing to mess with cfg(stage0) or something like that in order for that to work properly.
I can change it if you'd like, though--let me know.
| f? @erickt | 
77acf35    to
    8380d32      
    Compare
  
    | Eh, it looks fine enough for me. @bors r+ | 
| 📌 Commit 8380d32 has been approved by  | 
| ☔ The latest upstream changes (presumably #37246) made this pull request unmergeable. Please resolve the merge conflicts. | 
8380d32    to
    7bbebb1      
    Compare
  
    | Rebased. | 
| @bors r+ | 
| 📌 Commit 7bbebb1 has been approved by  | 
…ddyb Replace syntax's SmallVector with AccumulateVec This adds a new type to data_structures, `SmallVec`, which wraps `AccumulateVec` with support for re-allocating onto the heap (`SmallVec::reserve`). `SmallVec` is then used to replace the implementation of `SmallVector` in libsyntax. r? @eddyb Fixes rust-lang#37371. Using `SmallVec` instead of libsyntax's `SmallVector` will provide the `N = 2/4` case easily (just needs a few more `Array` impls). cc @nnethercote, probably interested in this area
| This is nice, thank you. I particularly like that you found a way to let N be choosable. E.g. I was looking at  | 
…ddyb Replace syntax's SmallVector with AccumulateVec This adds a new type to data_structures, `SmallVec`, which wraps `AccumulateVec` with support for re-allocating onto the heap (`SmallVec::reserve`). `SmallVec` is then used to replace the implementation of `SmallVector` in libsyntax. r? @eddyb Fixes rust-lang#37371. Using `SmallVec` instead of libsyntax's `SmallVector` will provide the `N = 2/4` case easily (just needs a few more `Array` impls). cc @nnethercote, probably interested in this area
Rollup of 30 pull requests - Successful merges: #37190, #37368, #37481, #37503, #37527, #37535, #37551, #37584, #37600, #37613, #37615, #37659, #37662, #37669, #37682, #37688, #37690, #37692, #37693, #37694, #37695, #37696, #37698, #37699, #37705, #37708, #37709, #37716, #37724, #37727 - Failed merges: #37640, #37689, #37717
This adds a new type to data_structures,
SmallVec, which wrapsAccumulateVecwith support for re-allocating onto the heap (SmallVec::reserve).SmallVecis then used to replace the implementation ofSmallVectorin libsyntax.r? @eddyb
Fixes #37371. Using
SmallVecinstead of libsyntax'sSmallVectorwill provide theN = 2/4case easily (just needs a few moreArrayimpls).cc @nnethercote, probably interested in this area