Improve Eq Hs instances
#348
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sheaf
reviewed
Jan 6, 2025
TravisCardwell
added a commit
that referenced
this pull request
Jan 6, 2025
The `Size` can be deduced from `Vec n NameHint`. This change moves the `induction` closer to where it is used.
edsko
approved these changes
Jan 8, 2025
phadej
reviewed
Jan 8, 2025
|
I think we shouldn't need Eq instances at all. This is all ugly and tedious code, and also error prone one. |
|
@phadej I think we got started on this path due to the |
|
@edsko, yes, https://hackage.haskell.org/package/tree-diff-0.3.4 does exactly that. I feel ashamed that I didn't notice that (and disappointed that nobody else either) for the seven years package existed. |
I received feedback about how `Vec.withDict` can be used to recover the `SNatI` dictionary of a `Vec`, and using it makes adding `SNatI` many constraints unnecessary. Thank you @sheaf! I also received feedback about using `GEq`. Thank you @phadej! This commit refactors the changes made to commit "Add Hs AST Eq instances, test using tree-diff" (`916ab6db`). It removed all added `SNatI` constraints and uses `GEq` instances. An unsightly aspect of the refactored code is the use of auxiliary functions just to bring type variables that are not part of the type signature in scope, but I do not know of a way to do this otherwise.
Based on feedback/guidance from @sheaf, `equals` from `HsBindgen.Util.TestEquality` and a new `ApEq` wrapper are used to significantly simplify the instance definitions. With a new `geqVec` function, auxiliary functions that just bring type variables into scope are no longer needed. Thank you @sheaf!
TravisCardwell
force-pushed
the
hs-eq-instances
branch
from
86fb276 to
5243b33
Compare
|
Implementing the Thank you for the update to |
TravisCardwell
added a commit
that referenced
this pull request
Jan 8, 2025
This commit removes the Eq instances that were added in the following commits: * `916ab6dc` Add Hs AST Eq instances, test using tree-diff * `a02d5d28` Improve Eq Hs instances * `5243b337` Improve Eq Hs instances (2) Note that `ApEq` in `HsBindgen.Util.TestEquality` is *not* removed. Discussion: #348
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
I received feedback about how
Vec.withDictcan be used to recover theSNatIdictionary of aVec, and using it makes addingSNatImany constraints unnecessary. Thank you @sheaf!I also received feedback about using
GEq. Thank you @phadej!This commit refactors the changes made to commit "Add Hs AST Eq instances, test using tree-diff" (
916ab6db). It removed all addedSNatIconstraints and usesGEqinstances.An unsightly aspect of the refactored code is the use of auxiliary functions just to bring type variables that are not part of the type signature in scope, but I do not know of a way to do this otherwise.