rustdoc: Start cleaning up search index generation by camelid · Pull Request #92340 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation23 Commits8 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
The old name wasn't very clear, while the new one makes it clear that this is the code responsible for creating the search index.
Based on
rust-lang#80883 (comment).
The tcx parameters do seem to be used though, so I only removed the
cache parameters.
It was previously defined in render::search_index but wasn't used at
all there. clean::types seems like a better fit since that's where
ExternalCrate is defined.
Now the only two crate-public items are build_index and
get_index_search_type (because for some reason the latter is also used
in formats::cache).
Some changes occurred in clean/types.rs.
cc @camelid
Relevant to the rustdoc team, which will review and decide on the PR/issue.
label
I recommend reviewing this commit-by-commit.
| ) { |
|---|
| fn insert_ty( |
| res: &mut Vec, |
| tcx: TyCtxt<'_>, |
| ty: Type, |
| mut generics: Vec, |
| _cache: &Cache, |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is the right move because of:
} else if let Some(kind) = ty.def_id_no_primitives().map(|did| tcx.def_kind(did).into()) {
We are replacing def_id_no_primitives calls, so we need to keep the Cache.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I see what you mean, but the Cache can easily be re-added later. As of now, passing around the Cache is just dead code that might be sitting there for a while.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reviewed a PR doing exactly that today (but of course, can't find it again...). I'd prefer to avoid having removals/adds following but not sure it matters much since it's just about git history here...
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're thinking of #92342? I'd rather remove the dead code for now – e.g., that PR may not work and end up being closed, and then the dead code will still be there.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't fully agree with you but it's fine. Let's move forward then!
| @@ -378,10 +378,12 @@ fn get_real_types<'tcx>( |
|---|
| /// i.e. `fn foo<A: Display, B: Option>(x: u32, y: B)` will return |
| /// `[u32, Display, Option]`. |
| fn get_all_types<'tcx>( |
| generics: &Generics, |
| decl: &FnDecl, |
| func: &Function, |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
| clean::BorrowedRef { ref type_, .. } => get_index_type_name(type_, accept_generic), |
|---|
| clean::Generic(_) |
| | clean::BareFunction(_) |
| clean::BorrowedRef { ref type_, .. } => get_index_type_name(type_), |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch!
Overall, this is really great, thanks! Just one change that would be problematic and I think it'd be better to remove it.
This comment has been minimized.
This issue was fixed using a hacky recursion "fuel" argument, but the
issue was never minimized nor was a regression test added. The
underlying bug is still unfixed, so this test should help with fixing it
and removing the recurse hack.
📌 Commit 908a9d4 has been approved by GuillaumeGomez
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
bors added a commit to rust-lang-ci/rust that referenced this pull request
…askrgr
Rollup of 7 pull requests
Successful merges:
- rust-lang#92075 (rustdoc: Only special case struct fields for intra-doc links, not enum variants)
- rust-lang#92118 (Parse and suggest moving where clauses after equals for type aliases)
- rust-lang#92237 (Visit expressions in-order when resolving pattern bindings)
- rust-lang#92340 (rustdoc: Start cleaning up search index generation)
- rust-lang#92351 (Add long error explanation for E0227)
- rust-lang#92371 (Remove pretty printer space inside block with only outer attrs)
- rust-lang#92372 (Print space after formal generic params in fn type)
Failed merges:
r? @ghost
@rustbot modify labels: rollup
Labels
Area: Rustdoc's search feature
Category: PRs that clean code up or issues documenting cleanup.
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Relevant to the rustdoc team, which will review and decide on the PR/issue.