Skip to content

Extract overlap_check_considering_specialization from insert #108577

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

spastorino
Copy link
Member

I think this or something along the lines the proposed changes is a bit simpler to read. Also I'm starting to work on #102678 and I don't want to keep mutating the insert method :).

r? @compiler-errors

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 28, 2023
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if I understand the motivation for this refactor. Are you using overlap_check_considering_specialization somewhere else? I'd prefer if you stacked this commit into some other PR where it's actually being used.

}

Ok(Inserted::BecameNewSibling(last_lint))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Inserted" makes no sense anymore, because we're not inserting anything until after this fn returns.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've renamed Inserted to OverlapResult and changed all the variant names. I think it makes a bit more sense now but maybe still improvable.

@spastorino spastorino force-pushed the extract-overlap-check-from-insert branch from 5cba984 to a73d57b Compare February 28, 2023 20:14
@spastorino
Copy link
Member Author

spastorino commented Feb 28, 2023

I'm not sure if I understand the motivation for this refactor. Are you using overlap_check_considering_specialization somewhere else? I'd prefer if you stacked this commit into some other PR where it's actually being used.

It's just to extract code that exist to do overlap check and specialization checks that lives inline inside insert. My plan was to land this first and then open a new PR to change how coherence overlap check works to fix #102678.

Anyway, we can land everything in one bigger PR if you prefer.

@compiler-errors
Copy link
Member

Let's wait until the follow-up PR that actually uses this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants