Created on 2019-07-29.16:33:52 by bfrk, last changed 2019-08-06.11:16:08 by ganesh.
See mailing list archives
for discussion on individual patches.
msg21004 (view) |
Author: bfrk |
Date: 2019-07-29.16:33:51 |
|
Random selection of cleanups and minor refactors.
11 patches for repository http://darcs.net/screened:
patch ad404366d882ccc8e318bc0a35fbd20098f53f81
Author: Ben Franksen <ben.franksen@online.de>
Date: Tue Jul 9 18:32:54 CEST 2019
* tentativelyMergePatches: rename anonpend to anonpw for consistency
patch a0bdfd080a49e6f0f7595af1ba209b331d2d6653
Author: Ben Franksen <ben.franksen@online.de>
Date: Sun Jul 14 13:45:34 CEST 2019
* clean up imports in two modules
patch aa9698db53abd5b29ad88a71c9380677b05f9399
Author: Ben Franksen <ben.franksen@online.de>
Date: Tue Jul 16 19:24:54 CEST 2019
* add 2 TODO items with V3INTEGRATION tag to Darcs.Util.Graph
patch 432e3b65104266ed2fa8346b704ac9ee32256dab
Author: Ben Franksen <ben.franksen@online.de>
Date: Wed Jul 17 14:44:05 CEST 2019
* replace Darcs.Patch.Witnesses.Sealed.Stepped with Data.Functor.Compose
patch 62c407b37a318fab5813084ada1d67de83f7bfb4
Author: Ben Franksen <ben.franksen@online.de>
Date: Wed Jul 17 17:36:08 CEST 2019
* harness: add explanation to an XXX comment
patch bfff45457a20c3108180071d2c50fa54806a8c9f
Author: Ben Franksen <ben.franksen@online.de>
Date: Thu Jul 18 21:18:05 CEST 2019
* cleanup in an import of D.P.TouchesFiles
patch b7142aa256af354195ac06a05128f2837dbb63c5
Author: Ben Franksen <ben.franksen@online.de>
Date: Fri Jul 19 09:48:34 CEST 2019
* amend: make options local to definition of DarcsCommand
This saves lots of rather verbose type signatures. We did this a while ago
for most of the other commands.
patch 6293f0deb71753f493ddf5dfb9ab9d8f20bb2bde
Author: Ben Franksen <ben.franksen@online.de>
Date: Sat Jul 20 14:25:30 CEST 2019
* cleanup layout of doActualRecord
patch 0c7bcfcc9762ed26218a0d0561ea04984f3a83a4
Author: Ben Franksen <ben.franksen@online.de>
Date: Sun Jul 28 21:24:27 CEST 2019
* simplify D.P.V1.Prim and D.P.V2.Prim
GeneralizedNewtypeDeriving can nowadays derive more instances: Apply,
CommuteNoConflicts, and Show. Also remove unused extension
StandaloneDeriving, simplify instance ReadPatch for V1 (similar to the one
for V2), and uniformly rename FileNameFormat parameter to 'fmt'.
patch 9ac4e8f19cc5e62b9a7aedad956e6b97a6558519
Author: Ben Franksen <ben.franksen@online.de>
Date: Sun Jul 28 21:38:06 CEST 2019
* rename data constructors of FileNameFormat
patch c79cc65abec9b81565c804acade2a9b355f023fd
Author: Ben Franksen <ben.franksen@online.de>
Date: Sat Jul 13 11:25:34 CEST 2019
* some code cleanups for unsuspendCmd
This factors out the check that there are no unrecorded changes into a local
procedure that returns a Repository with new witnesses if successful, or
else dies. We also pass the command name to 'unsuspendCmd' to improve the
error message for the reify command and remove a number of pattern type
signatures that are no longer needed.
Attachments
|
msg21026 (view) |
Author: ganesh |
Date: 2019-08-05.13:55:48 |
|
> Author: Ben Franksen <ben.franksen@online.de>
> Date: Tue Jul 9 18:32:54 CEST 2019
> * tentativelyMergePatches: rename anonpend to anonpw for
> consistency
OK
> Author: Ben Franksen <ben.franksen@online.de>
> Date: Sun Jul 14 13:45:34 CEST 2019
> * clean up imports in two modules
OK
> Author: Ben Franksen <ben.franksen@online.de>
> Date: Tue Jul 16 19:24:54 CEST 2019
> * add 2 TODO items with V3INTEGRATION tag to Darcs.Util.Graph
OK
> Author: Ben Franksen <ben.franksen@online.de>
> Date: Wed Jul 17 14:44:05 CEST 2019
> * replace Darcs.Patch.Witnesses.Sealed.Stepped with
> Data.Functor.Compose
OK
> Author: Ben Franksen <ben.franksen@online.de>
> Date: Wed Jul 17 17:36:08 CEST 2019
> * harness: add explanation to an XXX comment
OK
> Author: Ben Franksen <ben.franksen@online.de>
> Date: Thu Jul 18 21:18:05 CEST 2019
> * cleanup in an import of D.P.TouchesFiles
OK
> Author: Ben Franksen <ben.franksen@online.de>
> Date: Fri Jul 19 09:48:34 CEST 2019
> * amend: make options local to definition of DarcsCommand
OK (though I'd like to find a better solution, I think we've
discussed it before)
> Author: Ben Franksen <ben.franksen@online.de>
> Date: Sat Jul 20 14:25:30 CEST 2019
> * cleanup layout of doActualRecord
OK (I assume no code changes)
> Author: Ben Franksen <ben.franksen@online.de>
> Date: Sun Jul 28 21:24:27 CEST 2019
> * simplify D.P.V1.Prim and D.P.V2.Prim
>
> GeneralizedNewtypeDeriving can nowadays derive more instances:
> Apply,
> CommuteNoConflicts, and Show. Also remove unused extension
> StandaloneDeriving, simplify instance ReadPatch for V1 (similar
> to the one
> for V2), and uniformly rename FileNameFormat parameter to 'fmt'.
It's not clear from the code which way the classes are now being
derived. Could this be clarified with DerivingStrategies?
https://gitlab.haskell.org/ghc/ghc/wikis/commentary/compiler/derivin
g-strategies
If we really are switching to GeneralizedNewtypeDeriving that would
be a behaviour change from the existing code for Show. The old
instance is roughly the same as "deriving stock". I think that's
probably better for debugging purposes.
> Author: Ben Franksen <ben.franksen@online.de>
> Date: Sun Jul 28 21:38:06 CEST 2019
> * rename data constructors of FileNameFormat
OK
|
msg21030 (view) |
Author: ganesh |
Date: 2019-08-05.14:27:21 |
|
> patch c79cc65abec9b81565c804acade2a9b355f023fd
> Author: Ben Franksen <ben.franksen@online.de>
> Date: Sat Jul 13 11:25:34 CEST 2019
> * some code cleanups for unsuspendCmd
>
> This factors out the check that there are no unrecorded changes
> into a local procedure that returns a Repository with
> new witnesses if successful, or else dies.
OK. I'm not sure which style is superior, using IsEq <- ... to prove
wR = wU for the rest of the do-block, or returning a new repository
value, but I don't feel too strongly.
> We also pass the command name to 'unsuspendCmd' to improve the
> error message for the reify command and remove a number of
> pattern type signatures that are no longer needed.
Looks good.
|
msg21033 (view) |
Author: bfrk |
Date: 2019-08-05.17:04:49 |
|
>> * some code cleanups for unsuspendCmd
>>
>> This factors out the check that there are no unrecorded changes
>> into a local procedure that returns a Repository with
>> new witnesses if successful, or else dies.
>
> OK. I'm not sure which style is superior, using IsEq <- ... to prove
> wR = wU for the rest of the do-block, or returning a new repository
> value, but I don't feel too strongly.
Good point.
Using IsEq <- ... in a do block is perfectly fine with me. It just
didn't occur to me that we could do that without having to re-introduce
the ugly pattern type signatures. I just tried it and found they are not
needed at all. And since the procedure in question doesn't modify the
repo in any way, I think returning an EqCheck here is actually better.
Will send a patch.
|
msg21035 (view) |
Author: bfrk |
Date: 2019-08-05.21:22:13 |
|
Following up on review.
1 patch for repository http://darcs.net/screened:
patch 62d9b1dbc38e9f9fe7d7e6d2884e9833b00b019c
Author: Ben Franksen <ben.franksen@online.de>
Date: Mon Aug 5 19:10:36 CEST 2019
* return EqCheck instead of Repository from requireNoUnrecordedChanges
Attachments
|
msg21036 (view) |
Author: bfrk |
Date: 2019-08-05.22:16:12 |
|
>> * amend: make options local to definition of DarcsCommand
>
> OK (though I'd like to find a better solution, I think we've
> discussed it before)
Yes we did. I still think this is currently our best option.
>> * cleanup layout of doActualRecord
>
> OK (I assume no code changes)
Correct.
>> * simplify D.P.V1.Prim and D.P.V2.Prim
>>
>> GeneralizedNewtypeDeriving can nowadays derive more instances:
>> Apply,
>> CommuteNoConflicts, and Show. Also remove unused extension
>> StandaloneDeriving, simplify instance ReadPatch for V1 (similar
>> to the one
>> for V2), and uniformly rename FileNameFormat parameter to 'fmt'.
>
> It's not clear from the code which way the classes are now being
> derived. Could this be clarified with DerivingStrategies?
Interesting point. I didn't think of the ambiguity and merely assumed
that 'deriving Show' would produce the same effect whether or not
GeneralizedNewtypeDeriving is in effect or not.
> If we really are switching to GeneralizedNewtypeDeriving that would
> be a behaviour change from the existing code for Show. The old
> instance is roughly the same as "deriving stock". I think that's
> probably better for debugging purposes.
I agree. However,
https://downloads.haskell.org/~ghc/8.6.5/docs/html/users_guide/glasgow_exts.html#a-more-precise-specification
explicitly says that Read, Show, Typeable, or Data are always derived
using stock strategy, unless this is explicitly overridden.
For the other two classes, 'deriving newtype' is what happens by default
and again this is what we want.
We could clarify the strategy using DerivingStrategies. However, this
also places a burden on the developer: they have to think about which
strategy to use in which case. I think relying on the default is safer:
it was carefully chosen to be semantically compatible, so that merely
switching GeneralizedNewtypeDeriving on or off does not change the behavior.
|
msg21039 (view) |
Author: ganesh |
Date: 2019-08-06.09:40:05 |
|
On 05/08/2019 23:16, Ben Franksen wrote:
> We could clarify the strategy using DerivingStrategies. However, this
> also places a burden on the developer: they have to think about which
> strategy to use in which case. I think relying on the default is safer:
> it was carefully chosen to be semantically compatible, so that merely
> switching GeneralizedNewtypeDeriving on or off does not change the behavior.
My view is that this is something the developer should be explicitly
aware of, and that would also make reading the code easier. I'd rather
not have to remember the exact GHC rules to know what's going on.
That said, if you remember that stock beats everything in cases where it
would matter, then the rules do lead to warnings on other ambiguities:
https://downloads.haskell.org/~ghc/8.6.5/docs/html/users_guide/glasgow_exts.html#default-deriving-strategy
So it's not too important to me.
|
|
Date |
User |
Action |
Args |
2019-07-29 16:33:52 | bfrk | create | |
2019-07-29 16:34:47 | bfrk | set | status: needs-screening -> needs-review |
2019-08-05 13:55:49 | ganesh | set | status: needs-review -> review-in-progress messages:
+ msg21026 |
2019-08-05 14:27:22 | ganesh | set | messages:
+ msg21030 |
2019-08-05 17:04:49 | bfrk | set | messages:
+ msg21033 |
2019-08-05 21:22:13 | bfrk | set | files:
+ patch-preview.txt, return-eqcheck-instead-of-repository-from-requirenounrecordedchanges.dpatch, unnamed messages:
+ msg21035 |
2019-08-05 22:16:13 | bfrk | set | messages:
+ msg21036 |
2019-08-06 09:40:05 | ganesh | set | messages:
+ msg21039 |
2019-08-06 09:43:17 | ganesh | set | status: review-in-progress -> accepted-pending-tests |
2019-08-06 11:16:08 | ganesh | set | status: accepted-pending-tests -> accepted |
|