-
Notifications
You must be signed in to change notification settings - Fork 13.4k
Move SmallVector and ThinVec out of libsyntax #53085
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
350fee0
to
107a9f1
Compare
src/librustc_allocator/expand.rs
Outdated
}; | ||
use syntax_pos::Span; | ||
|
||
use {AllocatorMethod, AllocatorTy, ALLOCATOR_METHODS}; | ||
|
||
type SmallVector<T> = SmallVec<[T; 1]>; |
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.
Let's have a single definition of this, probably in data structures.
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 thought about this, but it might reduce the tweakability of SmallVec
for different modules; are we sure this size fits all?
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.
Not all modules will use it; SmallVec
will still be exported. Ideally we'd rename SmallVector
to OneVector
.
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.
Ok; I'll update and rebase soon.
107a9f1
to
7235e7d
Compare
I've been thinking it might be good to make the cross-cutting changes ~once now, do you want to also include the necessary changes from #51640 here? That is, we'd also:
Might as well go ahead and replace the smallvec impls with smallvec from crates.io) and enable the union feature. |
@Mark-Simulacrum I wouldn't want to spoil the fun for @gootorov 😄; in addition, my commit should make things easier for them, as it shows all the places where adjustments are needed. Being done with |
Okay, sounds good. Thanks! @bors r+ |
📌 Commit 7235e7d has been approved by |
@ljedrz I was thinking what to do with that @Mark-Simulacrum I've already replaced |
…=Mark-Simulacrum Move SmallVector and ThinVec out of libsyntax - move `libsyntax::util::SmallVector` tests to `librustc_data_structures::small_vec` - remove `libsyntax::util::SmallVector` - move `libsyntax::util::thin_vec` to `librustc_data_structures::thin_vec` Other than moving these data structures where they belong it allows modules using `SmallVector<T>` (`SmallVec<[T; 1]>`) to specify their own length (e.g. 8 or 32) independently from `libsyntax`.
🔒 Merge conflict This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again. How do I rebase?Assuming
You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial. Please avoid the "Resolve conflicts" button on GitHub. It uses Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Error message
|
7235e7d
to
9876e38
Compare
Rebased. |
@bors r+ |
📌 Commit 9876e38 has been approved by |
5937048
to
e5e6375
Compare
@bors r+ |
📌 Commit e5e6375 has been approved by |
⌛ Testing commit e5e6375 with merge 3923066a2c5f60b1006493e52615bddee69bf16f... |
💔 Test failed - status-travis |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Move SmallVector and ThinVec out of libsyntax - move `libsyntax::util::SmallVector` tests to `librustc_data_structures::small_vec` - remove `libsyntax::util::SmallVector` - move `libsyntax::util::thin_vec` to `librustc_data_structures::thin_vec` Other than moving these data structures where they belong it allows modules using `SmallVector<T>` (`SmallVec<[T; 1]>`) to specify their own length (e.g. 8 or 32) independently from `libsyntax`.
☀️ Test successful - status-appveyor, status-travis |
📣 Toolstate changed by #53085! Tested on commit 23f09bb. 💔 clippy-driver on windows: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk, @rust-lang/infra). |
Tested on commit rust-lang/rust@23f09bb. Direct link to PR: <rust-lang/rust#53085> 💔 clippy-driver on windows: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk, @rust-lang/infra). 💔 clippy-driver on linux: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk, @rust-lang/infra). 💔 rls on windows: test-pass → build-fail (cc @nrc, @rust-lang/infra). 💔 rls on linux: test-pass → build-fail (cc @nrc, @rust-lang/infra).
Did I accidentally break something again 🤔? |
|
Whoops; I thought these were a part of the tests too 🙈. |
Yeah, looks like Clippy is using ThinVec. @ljedrz looks like it would be a very easy fix to Clippy if you wanted to send a PR, but I'd ping @Manishearth or @oli-obk first since they might be on it already.
No, we let the tools get broken, it's not a big deal :-) |
@nrc I'd be happy to issue a fix, but in a few hours I'm going on holiday and will have no access to a PC for a few days, so I should probably leave it to others. |
libsyntax::util::SmallVector
tests tolibrustc_data_structures::small_vec
libsyntax::util::SmallVector
libsyntax::util::thin_vec
tolibrustc_data_structures::thin_vec
Other than moving these data structures where they belong it allows modules using
SmallVector<T>
(SmallVec<[T; 1]>
) to specify their own length (e.g. 8 or 32) independently fromlibsyntax
.