Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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] List matching impls on type aliases #112429
[rustdoc] List matching impls on type aliases #112429
Changes from all commits
4b1d13d
6f552c8
2ce7cd9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I'm sure threading this parameter through all these function calls is probably cheaper than using
let is_alias = tcx.def_kind(it) == DefKind::TyAlias
and usinglet aliased_type = tcx.type_of(it)
to resolve it, since the calling functions already have this information.But is pulling the information out of TyCtxt expensive enough to be worth having a function with seven parameters?
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.
We need a variable to tell us that this is an alias. From this, since we already have the
DefId
, why not using it directly?EDIT: Just re-read your comment. I'm not sure to like it much simply because we re-compute an information we already have. But that's pretty much my only argument against it. ^^'
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.
I edited my comment, I don't know how but I misread your comment the first time. Sorry about that.
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's not a lot of computation going into it, though. It's looking information up in a hash table.
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.
It's not the part worrying me but since I can't put the finger on it, I'll just remove the newly added argument and let's see how it goes. :)
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.
Ah found it! I need it because of
Some(aliased_type) => cache.impls.get(&aliased_type).unwrap_or(&empty),
. We can't retrieve theDefId
easily from aTy
. The only method I found to get it is ty_adt_id but it doesn't cover all the cases we need.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.
That makes sense. Thanks for the info!
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.
Having
aliased_type
separately here feels off to me and may at least deserve a comment, but that's unrelated to the type system itself so I am fine with that.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.
The doc comment for
aliased_type
is already on the function. ;)