-
Notifications
You must be signed in to change notification settings - Fork 13.7k
Add support for macro expansion in rustdoc source code pages #137229
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
Some changes occurred in HTML/CSS/JS. cc @GuillaumeGomez, @jsha These commits modify the If this was unintentional then you should revert the changes before this PR is merged. |
src/librustdoc/html/highlight.rs
Outdated
"{closing}\ | ||
<span class=expansion>\ | ||
<input id={id} type=checkbox>\ | ||
<label for={id}>↕</label>{reopening}", |
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.
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.
Because it would force to duplicate all the common code. For example in:
func(arg, macro!(blabla), arg3);
Only macro!(blabla)
will be different, so it's not interesting to duplicate all the rest. Instead, we put the original code in a span
, the expanded one in another and then we can simply switch them based on the status of the checked
property of the input. I'm also not a big fan of our current details
approach as it requires more JS to be working fully as expected but it's a debate for another time.
This comment has been minimized.
This comment has been minimized.
49a5ec0
to
9446a39
Compare
This comment has been minimized.
This comment has been minimized.
9446a39
to
f0bf946
Compare
f0bf946
to
faf64fa
Compare
This comment has been minimized.
This comment has been minimized.
This PR modifies cc @jieyouxu |
This comment has been minimized.
This comment has been minimized.
I opened the firefox bug report: https://bugzilla.mozilla.org/show_bug.cgi?id=1949948 |
☔ The latest upstream changes (presumably #137511) made this pull request unmergeable. Please resolve the merge conflicts. |
d54f0d0
to
a3e1a8b
Compare
This comment has been minimized.
This comment has been minimized.
a3e1a8b
to
8b1c7cb
Compare
8b1c7cb
to
b666b7f
Compare
PR is ready for review. |
☔ The latest upstream changes (presumably #140702) made this pull request unmergeable. Please resolve the merge conflicts. |
b666b7f
to
224ad51
Compare
Fixed merge conflict. |
224ad51
to
c9eed92
Compare
Went around firefox bug, so now it's always displayed as expected. |
42e6d1e
to
026687e
Compare
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
Applied code improvement suggestions, improved code comments and added a test with a macro coming from another file. |
This comment has been minimized.
This comment has been minimized.
026687e
to
f5fddc7
Compare
This comment has been minimized.
This comment has been minimized.
Seems like I didn't forget one a test this time. :') |
@bors r+ |
☀️ Test successful - checks-actions |
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing 3776358 (parent) -> 809200e (this PR) Test differencesShow 8 test diffsStage 1
Stage 2
Additionally, 6 doctest diffs were found. These are ignored, as they are noisy. Job group index Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard 809200ec956983fce4ae178b87dada69f01d0820 --output-dir test-dashboard And then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
Finished benchmarking commit (809200e): comparison URL. Overall result: ❌ regressions - please read the text belowOur benchmarks found a performance regression caused by this PR. Next Steps:
@rustbot label: +perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 3.1%, secondary 4.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 4.0%, secondary 14.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 467.391s -> 469.654s (0.48%) |
Sadly the perf impact was to be expected since it adds more code. |
If this option turned off by default, it shouldn't slowdown anything (at least over noise level), weird. |
It changed how the highlighting is working to handle the new case, and that is run all the time. I will need to rethink a bit how it works though because code could likely be better and less bad perf-wise. |
This is what it looks like:
You can test it here. In this case, I also enabled the
--generate-link-to-definition
to show that both options work well together.Note:
There is a bug currently in firefox where the line numbers are not displayed correctly if they're inside the "macro expansion" span: https://bugzilla.mozilla.org/show_bug.cgi?id=1949948Found a workaround around this bug.r? @notriddle