-
Notifications
You must be signed in to change notification settings - Fork 715
feat: Decidable as subtype of Bool
#8309
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
base: master
Are you sure you want to change the base?
Conversation
91e03d3 to
3bc08d6
Compare
|
Mathlib CI status (docs):
|
2f09103 to
9f39872
Compare
67a965f to
e827467
Compare
d1a4f37 to
ab73b1c
Compare
a1ed0b3 to
95b6237
Compare
e5e4415 to
87f3e3e
Compare
hargoniX
left a comment
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 general approach and the changes you did inside of the compiler seem reasonable to me. I do not feel confident enough in all of the elaborator to sign off on the change my own so I would prefer someone else to take a look at that as well.
One thing I was wondering though: You're redefining lots of Decidable instances though I would assume (or at least hope) that one can simply keep the old code around without type checking issues? What's the motivation for the change here?
|
Well, the idea is to make boolean functions and |
|
!radar |
|
Benchmark results for 23d879d against be2c2bc are in! @hargoniX Major changes (8)
Minor changes (18)
|
|
I wouldn't call this a |
Decidable as subtype of BoolDecidable as subtype of Bool
|
Yeah, you're right, I also thought about that |
|
I am not a maintainer or anything - so my voice has the least weight here compared to anyone else - but I do want to say I like these changes and find them very sensible in principle, and have some comments. One question: you mention that due to these changes, I also wonder about this |
|
Well, the main point of the |
|
My gut feeling is that one should only define something if one intends to create API for it. You could replace I see the point about proof irrelevancy making the match statements not matter though and bootstrapping can be a pain. My instinct is that |
You're right, that's what I originally had. However, |
|
Right, yes I see. If they weren't abbreviations that would not be an issue, is that right? |
This PR changes the definition of
Decidable pto a structure containing aBooland a proof of eitherpor¬p(basically the approach proposed by @kmill in #2038). Due to bugs in the old compiler, this was previously not possible; however, now that the new compiler is enabled, this works perfectly fine.Using
Boolin the definition ofDecidablehas several advantages, in particulardecidetactic no longer needs to carry proofs with it, improving performance for well-writtenDecidableinstances.LawfulBEqandDecidableEqare now compatible: When using theDecidableEqinstance provided byLawfulBEq,decide (a = b)is definitionally equivalent toa == b.Decidableno longer needs special casing in the compiler.In order to take full advantage from these changes, it is recommended to use the
decidable_of_boolanddecidable_of_ifffunctions to constructDecidableinstances.