Conversation
| -- to have mismatched behavior. | ||
| instance Eq Binder where | ||
| (==) NullBinder NullBinder = True | ||
| (==) NullBinder _ = False |
There was a problem hiding this comment.
This block of changes moves all lines like these to a final:
_ == _ = False
| NullBinder == NullBinder = | ||
| True | ||
| (LiteralBinder _ lb) == (LiteralBinder _ lb') = | ||
| lb == lb' |
There was a problem hiding this comment.
Not sure what the line-wrapping coding standard is. Could either:
- Use one line for each expression. Lines may grow long.
- Break after cutoff point, e.g. 80 characters. I feel this adds inconsistency to the structure.
- Break at each assignment. What's done in this PR.
There was a problem hiding this comment.
Breaking at each assignment like you've done here looks good to me. These things aren't really consistent across the compiler codebase at the moment.
| data DeclarationRef | ||
| -- | | ||
| -- A type constructor with data constructors | ||
| -- A type class |
There was a problem hiding this comment.
Reordered to match linting order
| -- enable sorting lists of explicitly imported refs when suggesting imports in linting, IDE, etc. | ||
| -- not an Ord because this implementation is not consistent with its Eq instance. | ||
| -- think of it as a notion of contextual, not inherent, ordering. |
There was a problem hiding this comment.
I don't think we need this comment anymore, since it now follows the order of the constructors.
| compareType (KindApp {}) _ = LT | ||
| compareType _ (KindApp {}) = GT | ||
|
|
||
| compareType (ForAll _ a b c d) (ForAll _ a' b' c' d') = compare a a' <> compareMaybeType b b' <> compareType c c' <> compare d d' |
There was a problem hiding this comment.
Here's an example of using long lines for each expression. This is another good candidate for the above coding style question.
|
Is there a reason for not defining the |
Maybe efficiency? I don't think we've ever measured it. |
|
As is, this change already removes a bunch of code so I'm happy to leave the question of whether we should define Eq via Ord for later. |
thomashoneyman
left a comment
thomashoneyman
left a comment
There was a problem hiding this comment.
👍 thanks for this work, @milesfrain!
5 participants
Follow-up to #3899 (comment)
Also applied the
orderOfstrategy to a few other places.Some additional discussion about strategies for skipping
SourceSpancomparisons in #3265