Skip to content

Refactor inhabited in exhaustivness check #3888

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

Merged
merged 2 commits into from
Jan 29, 2018

Conversation

liufengyun
Copy link
Contributor

Previously there is both implementability and inhabited. The refactoring merges the two and simplify the logic.

Copy link
Contributor

@OlivierBlanvillain OlivierBlanvillain left a comment

Choose a reason for hiding this comment

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

Code LGTM, @liufengyun could you give an intuition of why you need to compute this conjunctive normal form?

val tp1 = cnf(tp)
val l1 = cnf(l)
val r1 = cnf(r)
OrType(cnf(AndType(l1, tp1)), cnf(AndType(r1, tp1)))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you don't need to compute cnf(l), cnf(r) and cnf(tp) here, it's already handled by the recursion, the body of this case could be changed to the following:

OrType(cnf(AndType(l, tp)), cnf(AndType(r, tp)))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, good catch.

@liufengyun
Copy link
Contributor Author

@OlivierBlanvillain The motivation for CNF is to ease employing the facts of "single inheritance" and "final class". For checks that employ the fact about "singleton types", CNF is not required.

@liufengyun liufengyun merged commit 7799da6 into scala:master Jan 29, 2018
@liufengyun liufengyun deleted the refactor-inhabited branch January 29, 2018 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants