>> Author: Ben Franksen <ben.franksen@online.de>
>> Date: Sun Jun 30 20:18:37 CEST 2019
>> * simplify instances Eq2 for RebaseName and RebaseFixup
>
>> hunk ./src/Darcs/Patch/Rebase/Fixup.hs 69
>> - - PrimFixup _ `unsafeCompare` _ = False
>> - - _ `unsafeCompare` PrimFixup _ = False
>> - -
>> hunk ./src/Darcs/Patch/Rebase/Fixup.hs 70
>> - - -- NameFixup _ `unsafeCompare` _ = False
>> - - -- _ `unsafeCompare` NameFixup _ = False
>> + _ `unsafeCompare` _ = False
>> hunk ./src/Darcs/Patch/Rebase/Name.hs 138
>> - - AddName _ `unsafeCompare` _ = False
>> - - _ `unsafeCompare` AddName _ = False
>> - -
>> hunk ./src/Darcs/Patch/Rebase/Name.hs 139
>> - - DelName _ `unsafeCompare` _ = False
>> - - _ `unsafeCompare` DelName _ = False
>> - -
>> hunk ./src/Darcs/Patch/Rebase/Name.hs 140
>> - - -- Rename _ _ `unsafeCompare` _ = False
>> - - -- _ `unsafeCompare` Rename _ _ = False
>> + _ `unsafeCompare` _ = False
>
> The previous style here was actually deliberately verbose, to make
> sure we would get pattern match warnings if a new constructor is
> added. It's not a big deal, but I'd rather we roll this back.
(I see. A comment in the code would have prevented me from making this
change. I really wish there was a less verbose way to achieve the
desired effect, since it makes the code a lot harder to read. However,
this is obsoleted by the considerations below.)
I have spent some time today figuring out why these instances are needed
at all. I could not imagine rebase to depend on code that compares
things like RebaseChange or RebaseSelect for equality. Indeed this is
not the case.
The only instance that is actually used is the one for RebaseName. It is
needed so that in simplifyPush we can cancel out inverses. This is okay,
we can derive an instance Eq for RebaseName and define
instance Eq2 (RebaseName p) where
p1 =\/= p2
| p1 == unsafeCoercePEnd p2 = unsafeCoercePEnd IsEq
| otherwise = NotEq
We could as well implement isInverseOf directly for RebaseName and scrap
the instance.
All the other instances, including the ones for RebaseSelect and
RebaseChange aren't actually needed or used. They ultimately result from
a superclass constraint that I added to class Ident because I thought it
would be convenient to have that. I feared that without it I would have
had to add lots of extra Eq2 constraints everywhere.
However, I found that the opposite is the case! Removing this spurious
superclass constraint reduces the number of Eq2 constraints
considerably. We just need to add a handfull of extra Eq2 constraints,
mostly in properties in Darcs.Patch.Ident. But we can eliminate such
constraints for the instance Ident Named, and consequently everywhere in
Darcs.Patch.Match and almost everywhere in Darcs.Patch.Depends. This
allows it to be removed from getLogInfo, which in turn allows to
eliminate the Eq2 instances for RebaseFixup, RebaseSelect, and RebaseChange.
I think this is the best solution to the dilemma and I will send a patch
to that effect.
|
