Skip to content
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

rustdoc: generate JSON via serde #61028

Closed
wants to merge 1 commit into from
Closed

Conversation

euclio
Copy link
Contributor

@euclio euclio commented May 22, 2019

This PR replaces all usage of libserialize in rustdoc with serde and serde_json.

It also removes all derivations of RustcEncodable and RustcDecodable, which appear to be unused. This revealed some variants that cannot be constructed, so I removed those as well.

@@ -34,15 +34,12 @@ extern crate rustc_interface;
extern crate rustc_metadata;
extern crate rustc_target;
extern crate rustc_typeck;
extern crate serialize;
extern crate syntax;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm; unrelatedly to this PR, these extern crates seem unfortunate; would be good to get rid of them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't they necessary since they're being loaded from the sysroot?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly... I'm not familiar with how rustdoc operates. =P One could try to remove them and see what happens.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it loads all the rustc crates from the sysroot, not via --extern flags. Does rust 2018 allow you to implicitly load in sysroot crates?

@@ -2524,7 +2523,6 @@ pub enum TypeKind {
Struct,
Union,
Trait,
Variant,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this "some variants that cannot be constructed"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this and Type::Unique triggered the dead code lint.

@euclio
Copy link
Contributor Author

euclio commented May 26, 2019

r? @QuietMisdreavus

@@ -879,7 +889,7 @@ function handleThemeButtonsBlur(e) {{

themePicker.onclick = switchThemeButtonState;
themePicker.onblur = handleThemeButtonsBlur;
[{}].forEach(function(item) {{
{}.forEach(function(item) {{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took me a while to understand this change haha.

Copy link
Member

@GuillaumeGomez GuillaumeGomez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have a diff of the generated JS files to be sure we have exactly what's expected?

.collect::<Vec<String>>()
.join(",")).as_bytes(),
)?;
serde_json::to_string(&themes).unwrap(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it generate the same thing? Also, please replace unwrap with expect.

@bors
Copy link
Contributor

bors commented May 30, 2019

☔ The latest upstream changes (presumably #61343) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 30, 2019
@euclio
Copy link
Contributor Author

euclio commented Jun 6, 2019

Note to triage: I intend to return to this in the next week.

@euclio
Copy link
Contributor Author

euclio commented Jun 19, 2019

Differences in source-files.js and the implementors JavaScript are benign. I'll need to dig into the differences in the search index to confirm that they're OK too.

@bors
Copy link
Contributor

bors commented Jun 22, 2019

☔ The latest upstream changes (presumably #62041) made this pull request unmergeable. Please resolve the merge conflicts.

@euclio
Copy link
Contributor Author

euclio commented Jun 24, 2019

I am going to close this for now as I have just started a new job and I'm not sure what my schedule will be like the next few weeks. Hopefully I'll be able to resubmit this incrementally.

@euclio euclio closed this Jun 24, 2019
@euclio euclio deleted the rustdoc-serde branch July 4, 2019 00:50
bors added a commit that referenced this pull request Dec 13, 2019
replace serialize with serde in rustdoc

This is a slightly less aggressive version of #61028.

r? @GuillaumeGomez
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants