-
Notifications
You must be signed in to change notification settings - Fork 717
feat: make Option.decidableEqNone coherent with Option.instDecidableEq
#9302
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
|
|
||
| namespace Option | ||
|
|
||
| deriving instance DecidableEq for Option |
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.
Maybe this is a signal that we should be fixing the derive handler instead?
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'm not so sure; the current variant uses a single match which is usually sufficient. Doing double-match in general would probably just add unneeded complexity.
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.
Besides, the internals of DecidableEq is probably not something that should be relied upon.
|
Happy to merge this once the tests are green and we have a building mathlib. |
|
I have no idea why the build is failing, so I don't know how to continue here. |
|
Presumably you found it, but the relevant part of the log is |
|
Well, it's not actually equal under |
|
Mathlib CI status (docs):
|
|
Reference manual CI status:
|
|
Could you |
|
!bench |
|
Benchmark results for 8f37814 against 5cc0a10 are in! @robsimmons Runs (1)
|
8f37814 to
f48675b
Compare
Co-authored-by: Eric Wieser <[email protected]>
002c50e to
e9a9ee1
Compare
|
!bench |
|
Benchmark results for e9a9ee1 against 5a4226f are in! @robsimmons Runs (2)
|
|
!bench |
|
Benchmark results for 94a8559 against 5a4226f are in! @plp127 @robsimmons Major changes (1)
|
This PR adds support for decidable equality of empty lists and empty arrays. Decidable equality for lists and arrays is suitably modified so that all diamonds are definitionally equal. Following #9302, the strong condition of definitionally equal under `with_reducible_and_instances` is tested. This also moves some of the comments added in #9302 out of docstrings. --------- Co-authored-by: Aaron Liu <[email protected]> Co-authored-by: Eric Wieser <[email protected]>
This PR modifies
Option.instDecidableEqandOption.decidableEqNoneso that the latter can be made into a global instance without causing diamonds. It also addsOption.decidabeNoneEq.See Zulip.