-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
small coherence cleanup #74454
small coherence cleanup #74454
Conversation
// FIXME(eddyb) figure out if this is redundant with `ty_is_non_local`, | ||
// or maybe if this should be calling `ty_is_non_local_constructor`. |
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.
This FIXME
is interesting.
Will have to do something about this before we merge this PR
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.
Looking into the current impl it seems somewhat overly complicated but can't quite think of a clear solution here.
I do feel like using a TypeWalker
in fn orphan_check_trait_ref
might actually be the cleanest approach here, but iirc we wanted to slowly phase out uses of this struct.
What's the current status here?
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.
Will have to do something about this before we merge this PR
Is there an interaction, or you would just like to clean this up as part of the PR?
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 really would like to clean this up before merging this.
If we merge it without fixing this, we should at least update the method names 😅
r? @nikomatsakis (sorry, not sure what the plans are in this area) |
// FIXME(eddyb) figure out if this is redundant with `ty_is_non_local`, | ||
// or maybe if this should be calling `ty_is_non_local_constructor`. |
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.
Will have to do something about this before we merge this PR
Is there an interaction, or you would just like to clean this up as part of the PR?
@nikomatsakis this should be ready for merge/rereview. Wasn't able to improve |
@bors r+ |
📌 Commit cfcbca6 has been approved by |
small coherence cleanup r? @eddyb
…arth Rollup of 9 pull requests Successful merges: - rust-lang#73655 (va_args implementation for AAPCS.) - rust-lang#73893 (Stabilize control-flow-guard codegen option) - rust-lang#74237 (compiletest: Rewrite extract_*_version functions) - rust-lang#74454 (small coherence cleanup) - rust-lang#74528 (refactor and reword intra-doc link errors) - rust-lang#74568 (Apply rust-lang#66379 to `*mut T` `as_ref`) - rust-lang#74570 (Use forge links for prioritization procedure) - rust-lang#74589 (Update books) - rust-lang#74635 (Fix tooltip position if the documentation starts with a code block) Failed merges: r? @ghost
r? @eddyb