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
Improve or-with disjoint checks #7085
Improve or-with disjoint checks #7085
Changes from 11 commits
ad1c14b
470c6d5
c3225fa
2f4de50
3a861d9
891be6b
a7d57c7
133578a
0d50b8c
831e1c7
cfc634d
ec9e2b7
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.
Technically speaking to be complete this also needs
|| !self.with.is_disjoing(&self.without) || !other.with.is_disjoint(&other.without)
in order to handle extreme cases like(With<T>, Without<T>)
being empty and thus disjoint with everything else. Not sure if we want to properly support those cases though, in practice if they happen they're almost always a mistake. The condition being at the end means it would be checked only if we're about to find a conflict, so the performance hit should not happen in the happy path.However if it's kept like this I would add a comment explaining the missing conditions and why they're not implemented.
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.
Yeah, I was also thinking about this case. I'm inclined not to support it, I agree that it almost always means there's an error. Added a comment.