rustdoc: simplify highlight.rs#99337
Conversation
rustbot
added
the
T-rustdoc
rust-highfive
added
the
S-waiting-on-review
src/librustdoc/html/highlight.rs
Outdated
There was a problem hiding this comment.
This new distinction is really nice!
src/librustdoc/html/highlight.rs
Outdated
There was a problem hiding this comment.
This seems less good. Having to check if a string is empty is easy to forget whereas you cannot make the mistake with Option. Does it make sense?
src/librustdoc/html/highlight.rs
Outdated
There was a problem hiding this comment.
This is quite a big change. Until now the highlight depended on the edition provided to rustdoc. Even though it is a welcomed simplification, I'm not too sure if it's a good idea...
Another problem I have with this approach: we will need to not forget to update it whenever the edition is updated. To go around that limitation, you could add a current method on Edition which would return the current highest edition. To be discussed I suppose.
There was a problem hiding this comment.
Also, it seems it's not taking into account the edition parameter that the users can use on codeblocks.
src/librustdoc/html/sources.rs
Outdated
There was a problem hiding this comment.
Even though it could be simplified into Default::default(), I think it's personally much better this way. :)
I'm a bit worried about this. It's not a game changer but changes the way that some elements are (or not) highlighted. Maybe let's see what the others think? cc @rust-lang/rustdoc |
This comment has been minimized.
This comment has been minimized.
|
I mentioned the edition highlighting thing here: https://rust-lang.zulipchat.com/#narrow/stream/266220-rustdoc/topic/.60edition.60.20plumbed.20through.20highlighting.20code In tracing through the code, the only thing the edition is used for is to decide what is a keyword for the purpose of highlighting. The only place that matters is in the transition to Rust 2018: |
|
My point is that all these keywords were valid identifiers in the 2015 edition. So even if it's start getting old now, it's a change if we're highlighting them by default all the time now. Although, play.rust-lang.org highlights all of them (except for |
|
I’m also neutral about this change. Does anything other than rustdoc track the edition when syntax highlighting? What do rust-analyzer and intellij-rust do? |
rust-analyzer highlights |
|
Then I think it's fine to accept it. Just the code simplification makes it worth it. @jsha Please update your PR to fix the CI and then here we go! :) |
jsha
force-pushed
the
simplify-highlight
branch
from
d047b39 to
e0068d3
Compare
Split render_with_highlighting, which took many optional parameters, into three functions for specific purposes, which each take a smaller number of mostly required parameters. Remove some plumbing to pass through an "edition" parameter, which was used solely to avoid highlighting some 2021 Edition keywords in non-2021 code.
jsha
force-pushed
the
simplify-highlight
branch
from
e0068d3 to
5938fd7
Compare
|
Thanks for working on this! @bors r+ |
bors
added
S-waiting-on-bors
…eGomez rustdoc: simplify highlight.rs Split render_with_highlighting, which took many optional parameters, into three functions for specific purposes, which each take a smaller number of mostly required parameters. Remove some plumbing to pass through an "edition" parameter, which was used solely to avoid highlighting some 2021 Edition keywords in non-2021 code. I've tested a build of std docs before and after, and this does not change the generated HTML at all. Followup from rust-lang#91264 (comment) r? `@GuillaumeGomez`
…eGomez rustdoc: simplify highlight.rs Split render_with_highlighting, which took many optional parameters, into three functions for specific purposes, which each take a smaller number of mostly required parameters. Remove some plumbing to pass through an "edition" parameter, which was used solely to avoid highlighting some 2021 Edition keywords in non-2021 code. I've tested a build of std docs before and after, and this does not change the generated HTML at all. Followup from rust-lang#91264 (comment) r? ``@GuillaumeGomez``
…iaskrgr Rollup of 13 pull requests Successful merges: - rust-lang#93896 (rustdoc: make item-infos dimmer on dark theme) - rust-lang#99337 (rustdoc: simplify highlight.rs) - rust-lang#99421 (add crt-static for android) - rust-lang#99500 (Fix flags when using clang as linker for Fuchsia) - rust-lang#99511 (make raw_eq precondition more restrictive) - rust-lang#99992 (Add `x.sh` and `x.ps1` shell scripts) - rust-lang#100112 (Fix test: chunks_mut_are_send_and_sync) - rust-lang#100203 (provide correct size hint for unsupported platform `CommandArgs`) - rust-lang#100307 (Fix rust-lang#96847) - rust-lang#100350 (Stringify non-shorthand visibility correctly) - rust-lang#100374 (Improve crate selection on rustdoc search results page) - rust-lang#100392 (Simplify visitors) - rust-lang#100418 (Add stability attributes to BacktraceStatus variants) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Rollup of 7 pull requests Successful merges: - rust-lang#92744 (Check if enum from foreign crate has any non exhaustive variants when attempting a cast) - rust-lang#99337 (rustdoc: simplify highlight.rs) - rust-lang#100007 (Never inline Windows dtor access) - rust-lang#100030 (cleanup code w/ pointers in std a little) - rust-lang#100192 ( Remove duplicated temporaries creating during box derefs elaboration) - rust-lang#100247 (Generalize trait object generic param check to aliases.) - rust-lang#100374 (Improve crate selection on rustdoc search results page) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
7 participants
Split render_with_highlighting, which took many optional parameters, into three functions for specific purposes, which each take a smaller number of mostly required parameters.
Remove some plumbing to pass through an "edition" parameter, which was used solely to avoid highlighting some 2021 Edition keywords in non-2021 code.
I've tested a build of std docs before and after, and this does not change the generated HTML at all.
Followup from #91264 (comment)
r? @GuillaumeGomez