-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
save-analysis: Use serde instead of libserialize to dump JSON data #60053
Conversation
r? @varkor (rust_highfive has picked a reviewer for you, use r? to override) |
I tested the change by compiling rustc and using it to compile RLS, which seemed to work normally and also did spit the data using the new serde backend. FWIW I also tested running RLS on a crate which pulled in |
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.
👍
@bors: r+ |
📌 Commit 8498ba204b4c83c21f29d4918057a96cd601baef has been approved by |
Added a test, which checks that reading a malformed config doesn't panic @bors r=nrc |
📌 Commit b03123426c60dc22d84291a0fcecef67451514a8 has been approved by |
🔒 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
|
b031234
to
bc8af9a
Compare
That's weird... rebased just fine. @bors r=nrc |
📌 Commit bc8af9a has been approved by |
⌛ Testing commit bc8af9a with merge 227d009d96ba8703729c5ae049ddf0aec20041f9... |
💔 Test failed - checks-travis |
bc8af9a
to
4fb570d
Compare
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 |
Updated rls-* crates to explicitly not use @bors r=nrc |
📌 Commit 2dccaa7 has been approved by |
⌛ Testing commit 2dccaa7 with merge f51fc22e2711493012c954049be8c4c35c545968... |
💔 Test failed - checks-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 |
Argh, RLS broke again due to Clippy, will need to fix that first |
Looks like #60158 is almost merged, with
and the queue has only one PR (59114) pending without Cargo.lock, so I'll queue this up for the night: |
save-analysis: Use serde instead of libserialize to dump JSON data This breaks the save-analysis infrastructure (which also includes `rls-{analysis, data, span}` crates) from depending on rustc_serialize and so we can start moving them to being supported on stable without implementing `Decodable` et al. by hand for data structures defined there. Notable benefits: - we drop the awkward raw byte `PathBuf` [serialization](https://gist.github.com/Xanewok/f4fe8564d0dc0c3ab1dbc244279ff895) (until now (de)serialized as `&[u8]`) - [faster](https://github.com/serde-rs/json-benchmark) (hopefully noticeable for inner crate dependencies for the RLS workloads) - we can easily explore the binary serialization backend (which we planned to do for save-analysis anyway) ~This should be merged together with an update to RLS (rust-lang/rls#1435), which technically could be included right now because we can use the bundled `rls-analysis` here directly, however I'd prefer to publish this to crates.io first (rust-lang/rls#1434, cc @nrc) and use the published version, instead.~ Includes rust-lang/rls#1436. @matklad @nikomatsakis This is also important for the potential RLS 1.0 - 2.0 bridge we talked about on Zulip today
☀️ Test successful - checks-travis, status-appveyor |
📣 Toolstate changed by #60053! Tested on commit 8f06188. 💔 clippy-driver on windows: test-pass → test-fail (cc @Manishearth @llogiq @mcarton @oli-obk @phansch, @rust-lang/infra). |
Tested on commit rust-lang/rust@8f06188. Direct link to PR: <rust-lang/rust#60053> 💔 clippy-driver on windows: test-pass → test-fail (cc @Manishearth @llogiq @mcarton @oli-obk @phansch, @rust-lang/infra). 💔 clippy-driver on linux: test-pass → test-fail (cc @Manishearth @llogiq @mcarton @oli-obk @phansch, @rust-lang/infra).
This breaks the save-analysis infrastructure (which also includes
rls-{analysis, data, span}
crates) from depending on rustc_serialize and so we can start moving them to being supported on stable without implementingDecodable
et al. by hand for data structures defined there.Notable benefits:
PathBuf
serialization (until now (de)serialized as&[u8]
)This should be merged together with an update to RLS (rust-lang/rls#1435), which technically could be included right now because we can use the bundledrls-analysis
here directly, however I'd prefer to publish this to crates.io first (rust-lang/rls#1434, cc @nrc) and use the published version, instead.Includes rust-lang/rls#1436.
@matklad @nikomatsakis This is also important for the potential RLS 1.0 - 2.0 bridge we talked about on Zulip today