darcs

Patch 1845 fix typo in a comment (and 21 more)

Title fix typo in a comment (and 21 more)
Superseder Nosy List bf
Related Issues
Status accepted Assigned To
Milestone

Created on 2019-07-07.13:08:37 by bf, last changed 2019-08-06.11:00:06 by ganesh.

Files
File name Status Uploaded Type Edit Remove
eliminate-spurious-superclass-constraint-eq2-p-__-ident-p.dpatch bf, 2019-07-29.13:10:52 application/x-darcs-patch
fix-typo-in-a-comment.dpatch bf, 2019-07-07.13:08:36 application/x-darcs-patch
patch-preview.txt bf, 2019-07-07.13:08:36 text/x-darcs-patch
patch-preview.txt bf, 2019-07-29.13:10:52 text/x-darcs-patch
unnamed bf, 2019-07-07.13:08:36 text/plain
unnamed bf, 2019-07-29.13:10:52 text/plain
See mailing list archives for discussion on individual patches.
Messages
msg20887 (view) Author: bf Date: 2019-07-07.13:08:36
Assorted minor cleanups that came up during the last few weeks. Will
probably self-accept.

22 patches for repository http://darcs.net/screened:

patch b259ed0be04fe85aa3fd96eda54261bcf91c6420
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Jun 18 16:42:34 CEST 2019
  * fix typo in a comment

patch 659b97d0cf30e94fe5a0d4e18b8adf3b525e0259
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Jun 20 19:10:26 CEST 2019
  * harness: adapt test names for square commute law

patch 98401fdc043a962ab32113b9b729b23fbfd672ed
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Jun 21 15:30:40 CEST 2019
  * harness: remove unused propFail
  
  While a comment said this is "handy for debugging arbitrary code" I could
  not find an any further explanation or an example. I guess it can be
  re-added easily if needed.

patch 6862eaa84b66d17d63b8d55bccb8a041abb16b1d
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Jun 21 15:38:03 CEST 2019
  * harness: add some haddocks in D.T.P.A.Generic

patch 054cce01c44292de3ff2bd8f874b3fd2afbc061f
Author: Ben Franksen <ben.franksen@online.de>
Date:   Mon Jun 24 08:58:55 CEST 2019
  * v3: minor cleanups

patch 6bec24da8c50ec6db187501c0b8a90f67818e995
Author: Ben Franksen <ben.franksen@online.de>
Date:   Mon Jan 28 13:37:00 CET 2019
  * rename a parameter of promptChar

patch 529f46df41b1ede15d4ffdfb71607cdc8bb1654f
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Jun 27 16:52:07 CEST 2019
  * remove redundant cases for fastRemoveFL/RL

patch b0091a1eb868464a49b2026ea02c3b67884833c7
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Jun 28 08:39:24 CEST 2019
  * cleanup imports in D.P.Prim.Class

patch 63063860b7da15bb9116977f5139be69f9f597c0
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Jun 28 10:21:23 CEST 2019
  * break an overlong line in D.R.Merge

patch 0f91f32c818624fb8b70c927a8c0edab575d8e71
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Jun 28 10:23:38 CEST 2019
  * eliminate instance Merge for LabelledPatch and PatchChoice

patch efaca89dba06b22e2394a9a263e22216f84ddc23
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Jun 28 11:00:00 CEST 2019
  * remove unused eqFLRev

patch d0ceafb4f82172393861c70aac0bc8a76ecaa7c4
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Jun 30 10:58:01 CEST 2019
  * remove unneeded Eq2 instances from Darcs.Patch.Choices

patch 25a71cf056b17440240c59854d0c50887fa07c7d
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Jun 30 20:18:37 CEST 2019
  * simplify instances Eq2 for RebaseName and RebaseFixup

patch c718638779dc2770da80f5bbf5d663c5a12083ec
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Jun 30 21:24:39 CEST 2019
  * harness: remove two unneeded expression type signatures

patch 9791cd2a204751bbfe168b6059417f8455f37b49
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Jul  2 12:46:01 CEST 2019
  * fix in test script for issue2603

patch 81cfc0930ffe18f823a075f56c88970f1c1ca899
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Jul  5 18:32:59 CEST 2019
  * tests: minor fix and simplification in hidden_conflict2.sh

patch 6c3a5b6549881bdba8eb1f7f94347e5a433d7ce6
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Jul  7 08:40:25 CEST 2019
  * harness: comment about redundancy of mergeEitherWay

patch bd083052a67fb54ef2b861fc591168dbd9bc9a37
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Jun 30 20:08:02 CEST 2019
  * make PatchInfoAnd abstract

patch c434a449daa9112dbc726c8293f390952911e1d6
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Jun 21 15:35:52 CEST 2019
  * harness: remove some out-commented code from D.T.P.A.Generic

patch 8c11e88827e73c08e3a30f37385e347629fee92e
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Jul  7 13:42:05 CEST 2019
  * harness: remove unused sizeTree 

patch f28b597298931a8253a756ad34517fa5b98a16da
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Jul  7 08:37:53 CEST 2019
  * harness: better failure report for mergeEitherWayValid

patch 67519b823e14974253d669c4116649cf899fdf27
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Jul  7 08:26:07 CEST 2019
  * harness: minor cleanup in D.T.Patch
Attachments
msg20989 (view) Author: ganesh Date: 2019-07-28.14:04:05
> 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.

All the rest look good.
msg21001 (view) Author: bf Date: 2019-07-29.13:03:36
>> 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.
msg21002 (view) Author: bf Date: 2019-07-29.13:10:52
Following up on patch review.

1 patch for repository http://darcs.net/screened:

patch 5b16c72614972c77ca8eda5058a6f62eccac553e
Author: Ben Franksen <ben.franksen@online.de>
Date:   Mon Jul 29 15:08:35 CEST 2019
  * eliminate spurious superclass constraint Eq2 p => Ident p
  
  This change allows us to remove /lots/ of unneeded Eq2 constraints, and
  consequently a number of Eq2 instances under Darcs.Patch.Rebase.
Attachments
msg21041 (view) Author: ganesh Date: 2019-08-06.10:23:19
> (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.)

Good point re the comment. I had a look for any other instances
of the pattern and couldn't find any, but I'll try to remember to
add one if I do use it in future.

The follow-up patch looks good.
History
Date User Action Args
2019-07-07 13:08:37bfcreate
2019-07-07 16:09:05bfsetstatus: needs-screening -> needs-review
2019-07-28 14:04:05ganeshsetstatus: needs-review -> review-in-progress
messages: + msg20989
2019-07-29 13:03:36bfsetmessages: + msg21001
2019-07-29 13:10:52bfsetfiles: + patch-preview.txt, eliminate-spurious-superclass-constraint-eq2-p-__-ident-p.dpatch, unnamed
messages: + msg21002
2019-08-06 10:23:19ganeshsetstatus: review-in-progress -> accepted-pending-tests
messages: + msg21041
2019-08-06 11:00:06ganeshsetstatus: accepted-pending-tests -> accepted