darcs

Patch 1617 refactor and cleanup D.UI.SelectChanges and D.P.Choices

Title refactor and cleanup D.UI.SelectChanges and D.P.Choices
Superseder Nosy List bfrk
Related Issues
Status accepted Assigned To
Milestone

Created on 2017-10-10.11:01:55 by bfrk, last changed 2017-10-17.12:10:28 by gh.

Files
File name Status Uploaded Type Edit Remove
generalize-instance-invert-for-directed-pairs.dpatch bfrk, 2017-10-16.06:37:04 application/x-darcs-patch
removed-trash-from-d_ui_selectchanges.dpatch bfrk, 2017-10-10.11:01:54 application/x-darcs-patch
See mailing list archives for discussion on individual patches.
Messages
msg19723 (view) Author: bfrk Date: 2017-10-10.11:01:54
Yet another big fat bundle. It contains refactorings, cleanups, and
improved documentation for the (closely related) Darcs.UI.SelectChanges
and Darcs.Patch.Choices.

Note: there is more to come. My final goal is to make PatchChoices and
the way it is used in SelectChanges symmetric, so that when we start
patch selection from the end of the repo, moving backwards in time, we
no longer have to invert the patch sequences before the selection and
re-invert the results when we are done. I have made some progress in
this direction but what I have is not yet ready for sending.

26 patches for repository bf@hub.darcs.net:darcs-current:

patch 1cbe37c0ae85c0b3bf8333ce4945950148ac6b3e
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Oct  5 08:58:13 CEST 2017
  * removed trash from D.UI.SelectChanges

patch 2cb90ecac32feffd7d1247eb4fc1066cb0fe7b49
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Oct  5 09:13:35 CEST 2017
  * use mixed RL-FL catenation operators in D.UI.SelectChanges
  
  This saves us another bunch of unneeded reverseFL/RL calls.

patch 327c1330a701d8d421613fbe5fd3ebc501d277c9
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Oct  5 12:33:13 CEST 2017
  * break some overlong lines in askAboutDepends

patch f34058f3ab0beb249563dbe2bd484150a3076c2c
Author: Ben Franksen <benjamin.franksen@helmholtz-berlin.de>
Date:   Mon Aug 14 22:10:15 CEST 2017
  * renamed lpPatch to unLabel

patch 9865899627a6ae16a8a4f4b42a623cfa225696e8
Author: Ben Franksen <benjamin.franksen@helmholtz-berlin.de>
Date:   Fri Oct  6 16:30:26 CEST 2017
  * D.UI.SelectChanges: move functions definitions closer to their use

patch 0a6e388076c01e90ecd33a9ab0fbab1b654440ee
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Oct  6 17:41:47 CEST 2017
  * refactor D.UI.SelectChanges.runSelection

patch d2af680c1566668ac29deca5604be6f076acaa8c
Author: Ben Franksen <benjamin.franksen@helmholtz-berlin.de>
Date:   Fri Oct  6 18:53:01 CEST 2017
  * restrict pattern matching on WhichChanges to functions forward,
backward, and reversed

patch 251b159032fae1df58631fcdf76ea639c91888af
Author: Ben Franksen <benjamin.franksen@helmholtz-berlin.de>
Date:   Fri Oct  6 18:54:01 CEST 2017
  * renamed liftR to generalizeR and added a comment

patch e3d2349ac7cabb3f0fb91b258c3dbad199b9914d
Author: Ben Franksen <benjamin.franksen@helmholtz-berlin.de>
Date:   Fri Oct  6 19:31:47 CEST 2017
  * fixed layout of InteractiveSelectionContext

patch b16084afbf5b3661daa15c8b59fc80be1fa9c843
Author: Ben Franksen <benjamin.franksen@helmholtz-berlin.de>
Date:   Fri Oct  6 19:31:55 CEST 2017
  * pull calls to unLabel into repr

patch c7e969831bd80f8b64373e1a9ce92044bdc33316
Author: Ben Franksen <benjamin.franksen@helmholtz-berlin.de>
Date:   Fri Oct  6 19:33:03 CEST 2017
  * slightly improved haddocks for WhichChanges

patch 9b1c82345f09c1ff8e39d68cf4375fa663ba52be
Author: Ben Franksen <benjamin.franksen@helmholtz-berlin.de>
Date:   Fri Oct  6 20:06:58 CEST 2017
  * new code layout for Darcs.Patch.TouchesFiles

patch 6d6032a05bc52e27363c5950533eac18ee62dff5
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Oct  6 22:08:04 CEST 2017
  * refactor runSelection once more
  
  This patch concentrates all functions that are concerned with either
  pre-filtering the input patch list or post-filtering the selection
result in
  the where clause of runSelection. It turns out that these are all pure
  functions that can be combined using ordinary function composition.
This was
  previously obscured by the use of monadic style in the Reader monad, which
  we no longer need because the PatchSelectionContext is in scope inisde the
  where clause of runSelection.

patch 6a5046ea48e4828d748d467d8f9ea511064111eb
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat Oct  7 17:55:26 CEST 2017
  * fixed layout of some type dignatures in D.P.Choices

patch 90b58992bb21b0b4231b2661f3b6e1c09e0b7ea6
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat Oct  7 17:50:57 CEST 2017
  * replace patchChoicesLpsSub with labelPatches
  
  The left component of the pair was never used except in patchChoicesLps
  which we now implement more directly.

patch 1be8fc5a9ef0c915d3a0bb388a11034df781d105
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat Oct  7 18:39:31 CEST 2017
  * inlined patchSlot' (it is just state . patchSlot)

patch 7a1255ba9d5897f3225d0f153a4c65c5ddede90a
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat Oct  7 19:05:57 CEST 2017
  * removed forward from D.UI.SelectChanges
  
  The code is easier to modify if there is only one function that makes this
  distinction, and not two, so we now use backward throughout.

patch b1f1d242f60c18dfd755e332194e8c4d1057ae58
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat Oct  7 20:41:05 CEST 2017
  * renamed modChoices to modifyChoices in D.UI.SelectChanges

patch 155f3dbfe5e88dd942429ab7fc4e4e2d6804a415
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat Oct  7 20:31:38 CEST 2017
  * fix documentation of WhichChanges, backward, and reversed in
D.UI.SelectChanges

patch 65b9ab75007d30d4db87f2c5debdc5ea3a4f098c
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Oct  8 09:55:52 CEST 2017
  * export mkPatchChoices and inline patchChoicesLps, removing the latter

patch 3129d20e92bf7559afe46b8a2c00d88545cc29cb
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Oct  8 10:29:19 CEST 2017
  * cleanup type signatures in D.UI.SelectChanges

patch 04bfbbbaf8b2327ec73561de82173683c87de997
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat Oct  7 18:59:27 CEST 2017
  * fixed some comments in D.P.Choices

patch d1e1caaf9c70b1b6e3e24090295e5e903905e74a
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Oct  8 10:38:52 CEST 2017
  * remove instance Label Ord
  
  The instance is not needed and not having it makes it clear that user code
  is not supposed to rely on it. This give greater freedom if we want to
use a
  different representation at some point (e.g. unique IDs).

patch 632a9936d45a0f3e7b631ff29430f89e24524079
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Oct  8 10:43:18 CEST 2017
  * use Int instead of Integer for Labels
  
  Int should be large enough in practice and is a lot faster.

patch 5359f0cd3739cbea2ab6be9498f7fa261ac6e044
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Oct  8 15:50:36 CEST 2017
  * remove liftLP by inlining it into its single call site

patch cd97ce70f3e302ff8183b7feb38825850278c6f8
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Oct  8 15:55:27 CEST 2017
  * add lots of documentation to D.P.Choices
  
  The module is now fully haddock'd and cleaned up in many ways, but its
  functionality and implementation details have remained untouched. A
few very
  minor refactorings are included. I renamed some of the internal variables
  (mostly in an effort to understand the implementation better, which in
turn
  was necessary to write correct documentation), and also one or two of the
  exported functions to better fit with the rest of the module.
Attachments
msg19733 (view) Author: gh Date: 2017-10-11.15:50:22
TD;DR: Screened, and Accepted pending a followup patch that would
simplify the new Invert instance (see below).


* removed trash from D.UI.SelectChanges

OK (I would have used the term "commented code" :) )

  * use mixed RL-FL catenation operators in D.UI.SelectChanges
  
  This saves us another bunch of unneeded reverseFL/RL calls.

Yes!!

  * break some overlong lines in askAboutDepends

OK

  * renamed lpPatch to unLabel

Makes sense.

  * D.UI.SelectChanges: move functions definitions closer to their use

OK

  * refactor D.UI.SelectChanges.runSelection

Seems correct.


  * restrict pattern matching on WhichChanges to functions forward,
backward, and reversed

This simplifies many functions. OK.

  * renamed liftR to generalizeR and added a comment

OK

  * fixed layout of InteractiveSelectionContext

OK (also adds a comment)

  * pull calls to unLabel into repr

Good!!

  * slightly improved haddocks for WhichChanges

OK

  * new code layout for Darcs.Patch.TouchesFiles

OK

  * refactor runSelection once more
  
  This patch concentrates all functions that are concerned with either
  pre-filtering the input patch list or post-filtering the selection
result in
  the where clause of runSelection. It turns out that these are all pure
  functions that can be combined using ordinary function composition.
This was
  previously obscured by the use of monadic style in the Reader monad, which
  we no longer need because the PatchSelectionContext is in scope inisde the
  where clause of runSelection.


Seems correct. Just one thing.
In D.Patch.Invert, the following seems better to me:

~~~
    instance Invert p => Invert (p :> p) where
      invert (a :> b) = invert b :> invert a
~~~

  * fixed layout of some type dignatures in D.P.Choices

OK

  * replace patchChoicesLpsSub with labelPatches
  
  The left component of the pair was never used except in patchChoicesLps
  which we now implement more directly.

Good!

  * inlined patchSlot' (it is just state . patchSlot)

Good!


  * removed forward from D.UI.SelectChanges
  
  The code is easier to modify if there is only one function that makes this
  distinction, and not two, so we now use backward throughout.

Good!


  * renamed modChoices to modifyChoices in D.UI.SelectChanges

OK


  * fix documentation of WhichChanges, backward, and reversed in
D.UI.SelectChanges

OK

  * export mkPatchChoices and inline patchChoicesLps, removing the latter

OK (I find the code in Whatsnew obscure anyway).


  * cleanup type signatures in D.UI.SelectChanges

Yes! Death to the useless forall's and types in pattern matching.


  * fixed some comments in D.P.Choices

OK

  * remove instance Label Ord
  
  The instance is not needed and not having it makes it clear that user code
  is not supposed to rely on it. This give greater freedom if we want to
use a
  different representation at some point (e.g. unique IDs).

OK

  * use Int instead of Integer for Labels
  
  Int should be large enough in practice and is a lot faster.

Agreed, OK

  * remove liftLP by inlining it into its single call site

Good!

  * add lots of documentation to D.P.Choices
  
  The module is now fully haddock'd and cleaned up in many ways, but its
  functionality and implementation details have remained untouched. A
few very
  minor refactorings are included. I renamed some of the internal variables
  (mostly in an effort to understand the implementation better, which in
turn
  was necessary to write correct documentation), and also one or two of the
  exported functions to better fit with the rest of the module.
  

Great, I like the new comments.
msg19742 (view) Author: bfrk Date: 2017-10-12.08:34:15
> In D.Patch.Invert, the following seems better to me:
>
>     instance Invert p => Invert (p :> p) where
>       invert (a :> b) = invert b :> invert a

Yes, good catch. I hope it will not collide with the instances in the
harness (...compiling... okay no problem).

BTW, this instance

  instance Invert p => Invert (FL p) where
      invert = reverseRL . invertFL

makes me wonder: will the two traversals here be fused? Will they at
least be processed lazily, so that we don't have to build a complete
copy of the intermediate list in memory? (I don't believe so, see below).

Can we make sure they will be fused by adding appropriate rewrite RULES?
What about SPECIALIZE pragmas or INLINE?

And while we are at it: reverseFL/RL use an accumulator; they are tail
recursive. I may be wrong here, but I think this means they do not
process the FL/RL in a lazy fashion like foldr does for lists. How does
that influence the efficiency of pipelines like the above?

If they are like foldl, do they produce memory leaks? How strict is e.g.
inverting a Prim patch? We need benchmarks for these core data
structures and we also need some small examples so we can look at the
GHC core.

So much to do... sigh.
msg19753 (view) Author: bfrk Date: 2017-10-16.06:37:04
Added a second bundle with a patch that generalizes the instance Invert
for directed pairs as suggested by gh.
Attachments
msg19760 (view) Author: gh Date: 2017-10-17.12:10:28
Thanks!
History
Date User Action Args
2017-10-10 11:01:55bfrkcreate
2017-10-11 15:50:23ghsetstatus: needs-screening -> accepted-pending-tests
messages: + msg19733
2017-10-12 08:34:16bfrksetmessages: + msg19742
2017-10-16 06:37:04bfrksetfiles: + generalize-instance-invert-for-directed-pairs.dpatch
messages: + msg19753
2017-10-17 12:10:28ghsetstatus: accepted-pending-tests -> accepted
messages: + msg19760