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

Clean up rustdoc's main() #75124

Merged
merged 3 commits into from
Aug 5, 2020
Merged

Conversation

nnethercote
Copy link
Contributor

It can be simplified and made more similar to rustc's main().

r? @oli-obk

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 4, 2020
@nnethercote
Copy link
Contributor Author

@oli-obk: not sure if you're the best reviewer, I chose you because you did #55617 which is somewhat related. Feel free to forward the review to someone else if necessary.

@ollie27 ollie27 added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Aug 4, 2020
src/librustdoc/lib.rs Outdated Show resolved Hide resolved
src/librustdoc/lib.rs Outdated Show resolved Hide resolved
src/librustdoc/lib.rs Outdated Show resolved Hide resolved
rustdoc's `main()` immediately spawns a thread, M, with a large stack
(16MiB or 32MiB) on which it runs `main_args()`. `main_args()` does a
small amount of options processing and then calls
`setup_callbacks_and_run_in_default_thread_pool_with_globals()`, which
spawns it own thread, and M is not used further.

So, thread M seems unnecessary. However, it does serve a purpose: if the
options processing in `main_args()` panics, that panic is caught when M
is joined. So M can't simply be removed.

However, `main_options()`, which is called by `main_args()`, has a
`catch_fatal_errors()` call within it. We can move that call to `main()`
and change it to the very similar `catch_with_exit_code()`. With that in
place, M can be removed, and panics from options processing will still
be caught appropriately.

Even better, this makes rustdoc's `main()` match rustc's `main()`, which
also uses `catch_with_exit_code()`.

(Also note that the use of a 16MiB/32MiB stack was eliminated from rustc
in rust-lang#55617.)
It's a very thin wrapper around
`setup_callbacks_and_run_in_thread_pool_with_globals()` and it has a
single call site.
`run()` returns `Result<(), String>`. But on failure it always returns
an empty string, and then `wrap_return()` treats an empty string
specially, by not reporting the error.

It turns out we already have the `ErrorReported` type for this sort of
behaviour. This commit changes `run()` to use it.
@nnethercote
Copy link
Contributor Author

I have addressed all the comments.

@oli-obk
Copy link
Contributor

oli-obk commented Aug 5, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Aug 5, 2020

📌 Commit 5f8a112 has been approved by oli-obk

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 5, 2020
@bors
Copy link
Contributor

bors commented Aug 5, 2020

⌛ Testing commit 5f8a112 with merge 1d69e3b...

@bors
Copy link
Contributor

bors commented Aug 5, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: oli-obk
Pushing 1d69e3b to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 5, 2020
@bors bors merged commit 1d69e3b into rust-lang:master Aug 5, 2020
@nnethercote nnethercote deleted the clean-up-rustdoc-main branch August 5, 2020 23:50
@cuviper cuviper added this to the 1.47.0 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants