darcs

Patch 1907 Move PrimOf out of PrimPatchBase

Title Move PrimOf out of PrimPatchBase
Superseder Nosy List ganesh
Related Issues
Status in-discussion Assigned To
Milestone

Created on 2019-09-02.21:58:13 by ganesh, last changed 2019-09-18.16:56:01 by ganesh.

Files
File name Status Uploaded Type Edit Remove
move-primof-out-of-primpatchbase.dpatch ganesh, 2019-09-02.21:58:13 application/x-darcs-patch
move-primof-out-of-primpatchbase.dpatch ganesh, 2019-09-17.16:55:14 application/x-darcs-patch
patch-preview.txt ganesh, 2019-09-02.21:58:13 text/x-darcs-patch
patch-preview.txt ganesh, 2019-09-17.16:55:14 text/x-darcs-patch
unnamed ganesh, 2019-09-02.21:58:13 text/plain
unnamed ganesh, 2019-09-17.16:55:14 text/plain
See mailing list archives for discussion on individual patches.
Messages
msg21324 (view) Author: ganesh Date: 2019-09-02.21:58:13
For discussion initially.

It's inspired by the discussion about replacing Apply with
Effect+PrimApply as it would be necessary to break a
dependency cycle in that refactoring, but it makes sense
anyway.

It's also in keeping with the recent changes to make
PrimPatch and RepoPatch into type synonyms and the
design of Ident/PatchId.

1 patch for repository darcs-unstable@darcs.net:screened:

patch d91deaee5a0b51ae35db89e3b9a891612b27e5a7
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Mon Sep  2 23:00:01 BST 2019
  * Move PrimOf out of PrimPatchBase
  
  PrimPatchBase becomes a type synonym, and could perhaps
  just be removed altogether.
Attachments
msg21335 (view) Author: bf Date: 2019-09-03.13:04:54
Constraint synonyms have one disadvantage that I wasn't aware of until
recently. A superclass constraint can include a constraint on a type
family application e.g.

  class PrimApply (PrimOf p) => Whatever p

But of you use such a constraint for an instance (directly or via a
constraint synonym), then GHC complains and tells us it needs
UndecidableInstances.

It is interesting that this doesn't seem to apply to equality
constraints, i.e. Applystate p ~ ApplyState (PrimOf p) is okay, even on
an instance.

I am not happy about having to enable UndecidableInstances all over the
place. We need to find a better solution. These modules have it ATM:

src/Darcs/Patch/ApplyMonad.hs
src/Darcs/Patch/Rebase/Fixup.hs
src/Darcs/Patch/Rebase/Container.hs
src/Darcs/Patch/Rebase/Viewing.hs
src/Darcs/Patch/Rebase/Item.hs
src/Darcs/Patch/V2/Non.hs
src/Darcs/Patch/V2/Prim.hs
src/Darcs/Patch/V1/Prim.hs

I think all of them are due to the same issue above i.e. we need to have
"Class (TypeFunction p) => instance Whatever p".
msg21336 (view) Author: ganesh Date: 2019-09-03.13:52:27
> Constraint synonyms have one disadvantage that I wasn't aware of until
> recently. A superclass constraint can include a constraint on a type
> family application e.g.
> 
>   class PrimApply (PrimOf p) => Whatever p
> 
> But of you use such a constraint for an instance (directly or via a
> constraint synonym), then GHC complains and tells us it needs
> UndecidableInstances.

Ah yes, I meant to mention that. I actually thought some of the code
that uses RepoPatch/PrimPatch needed it too after the synonym was
introduced, but perhaps I was wrong.

> I am not happy about having to enable UndecidableInstances all over the
> place. We need to find a better solution. These modules have it ATM:

I guess that it's either use UndecidableInstances or keep the class
(with no members).
msg21440 (view) Author: ganesh Date: 2019-09-17.16:55:14
Here's an updated version that preserves PrimPatchBase,
so doesn't require any more UndecidableInstances.

Ben, what do you think?

1 patch for repository darcs-unstable@darcs.net:screened:

patch c86f011113412fc60b579238d19f8508ba5d4a19
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Tue Sep 17 17:56:29 BST 2019
  * Move PrimOf out of PrimPatchBase
  
  Quite a few PrimPatchBase instances then turn out to be
  unnecessary.
Attachments
msg21443 (view) Author: bf Date: 2019-09-18.07:17:53
> Here's an updated version that preserves PrimPatchBase,
> so doesn't require any more UndecidableInstances.
> 
> Ben, what do you think?

I am undecided. I don't see a clear win doing this but neither do I see
us losing anything. If you feel this is an improvement or if it helps
you with some other refactor, go ahead, no objections. Otherwise I'd say
let's save this until we have a pressing need. I am fine either way.
msg21444 (view) Author: ganesh Date: 2019-09-18.07:26:47
> I am undecided. I don't see a clear win doing this but neither do I see
> us losing anything. If you feel this is an improvement or if it helps
> you with some other refactor, go ahead, no objections. Otherwise I'd say
> let's save this until we have a pressing need. I am fine either way.

OK, I'm leaning towards having it because reducing the number of
PrimPatchBase instances seems nice. But I'll leave it for a bit to see
if I still think it's a good idea.

It is a pre-requisite for patch1914 but I think the merits of that are
very much in doubt.
msg21445 (view) Author: bf Date: 2019-09-18.16:49:13
> It is a pre-requisite for patch1914 but I think the merits of that are
> very much in doubt.

Do you really mean patch1914? (which is in screened)
msg21447 (view) Author: ganesh Date: 2019-09-18.16:56:01
On 18/09/2019 17:49, Ben Franksen wrote:
> 
> Ben Franksen <ben.franksen@online.de> added the comment:
> 
>> It is a pre-requisite for patch1914 but I think the merits of that are
>> very much in doubt.
> 
> Do you really mean patch1914? (which is in screened)

Sorry, patch1919.
History
Date User Action Args
2019-09-02 21:58:13ganeshcreate
2019-09-02 21:58:19ganeshsetstatus: needs-screening -> in-discussion
2019-09-03 13:04:55bfsetmessages: + msg21335
2019-09-03 13:52:27ganeshsetmessages: + msg21336
2019-09-17 16:55:14ganeshsetfiles: + patch-preview.txt, move-primof-out-of-primpatchbase.dpatch, unnamed
messages: + msg21440
2019-09-18 07:17:54bfsetmessages: + msg21443
2019-09-18 07:26:47ganeshsetmessages: + msg21444
2019-09-18 16:49:14bfsetmessages: + msg21445
2019-09-18 16:56:01ganeshsetmessages: + msg21447