Argh... Sorry, here is perhaps a more readable version. General comments and questions ------------------------------ 1) This does not compile with GHC 6.4.1, even without type witnesses. It complains about the constructors :/\: (and friends). Unless we can issue a fix rather quickly, we will have to require GHC 6.6 for the next release. Is that ok with you?
2) The new code will not parse/understand/ commute mergers. Is this correct? If so, I'm guessing you are planning some sort of repository conversion tool (à darcs optimize or upgrade). How do you reckon that would work? That is, what do mergers translate to? 3) I was wondering if there really exists such a thing as an empty composite or split patch (NilFL). Since we now have control over our own list type, would it be worthwhile to have lists that are guaranteed to have at least one element in them? You seem to be using ComP NilFL to double as an 'identity patch' (is that used just for internal purposes?). If this is the sole use of empty patch lists, maybe have a constructor just for that? Pattern guards -------------- > + p | IsEq <- nullP p -> id > + | otherwise -> (flatten p ++) More a comment for others. At first I didn't understand this code. Then I found out about the pattern guard extension: http://www.haskell.org/ghc/docs/latest/html/users_guide/syntax-extns.html Basically, code like myFn | IsEq <- nullP p = foo | otherwise = bar is a handier way to express something like myFn = case nullP p of IsEq -> foo _ -> bar This lightens up the code in some places. unsafeCoerce# ------------- > - p1_modifies /= p2_modifies = Succeeded (p2 :< p1) > + p1_modifies /= p2_modifies = Succeeded (unsafeCoerce# p2 :< > unsafeCoerce# p1) As I understand it, the unsafeCoerce#s are used for forcing the type witnesses. I wonder then if we could have a somewhat safer wrapper like unsafeCoercePatch :: Patch C(x,y) -> Patch C(a,b) Just to avoid, for example, unintentionally coercing something completely unrelated because of a bracketing typo. MergeFunction ------------- > +new_merge :: (Patch :\/: Patch) C(x,y) -> Maybe ((Patch :/\: Patch) C(x,y)) > +new_merge (p1:\/:p2) = do ip1' :< p2' <- commute (p2 :< invert p1) > + return (p2' :/\: invert ip1') Would a MergeFunction type be useful? type MergeFunction = (Patch :\/: Patch) C(x,y) -> Maybe ((Patch :/\: Patch) C(x,y)) Canonize -------- > - liftM2 (merger g) (canonize p1) (canonize p2) > + (merger g) (canonize p1) (canonize p2) I am somewhat concerned about this. As I understand it, we replace the Nothings with an identity patch (ComP NilFL). Fine. But in places like this, does this mean we lose this idea of failures propagating in the Maybe monad? For example, does the example above really behave the same way (or does it really not matter?). I confess that I wasn't able to figure what returning Nothing means. Does it mean that canonization "fails" in some way? > -canonize p@(FP _ (Binary old new)) = if old /= new then Just p > - else Just null_patch Another thing is that now Just null_patch and Nothing are collapsed into a single type of result, the identity patch. Is that ok? Conflicted ---------- (with David's modifications) A Conflicted patch is (as I understand Jason's mail) a storage mechanism on top of which the cancellation patches will be implemented. A conflicted patch consists of a patch and a sequence of patches (a patch context). I'm guessing that we call it this because it is something we generate when there is a conflict. Jason, can you explain to me what the relationship is between the two? For example, why don't we just have a list of patches? Conflicted patches always commute, although how they commute depends on what they commute with. There are three cases that are looked at in order 1. we are commuting with another Conflicted patch trivial: just do it 2. we try commuting with something that does not conflict with us (Conflicted p1 cs) :> p2 ok... p1 p2' cs' p2'' p1' cs' and we return p2'' :> (Conflicted p1' cs') Yeah, this is redundant, but it sometimes helps me to just work through things 3. we try commuting with something that _does_ conflict with us swallow it (black magic) > -- If the confilcted patch or the context does not commute with the typo :-) > -- other patch then we need to add the other patch to the context of > -- the conflicted patch. > -- The hard case here, is doing the inverse commute. To work correctly > -- we must make sure that the context has the correct patch at the end. > -- Otherwise we cannot find it to remove it from the context. > conflicted_commute_depends :: CommuteFunction > conflicted_commute_depends (Conflicted p1 csp2 :< p2) | > ((lastc:<:initcs):_) <- filter (\(lastc:<:_) -> isEq (p2 =/\= lastc)) $ > last_permutations csp2 > = case p2 =/\= lastc of > IsEq -> Succeeded (p2 :< Conflicted p1 (reverseRL initcs)) > _ -> impossible > conflicted_commute_depends (Conflicted p1 cs :< ip2) = > Succeeded (ip2 :< Conflicted p1 (cs+>+invert ip2:>:NilFL)) > conflicted_commute_depends _ = Unknown I haven't really tried to understand this code. Sorry. By the way, could you explain what the Proof stuff is for? Just for grabbing patches which don't modify context, for example, Conflicted patches? I'm guessing filterE would still have a use even though it is no longer in this code? Sealed ------ > +data Sealed a where > + Sealed :: !(a C(x,)) -> Sealed a Thanks to Ian and Ganesh's gracious help, I was able to make more sense of this code. My stumbling block was forgetting that you could curry type parameters, so I was mentally substituting Sealed :: Patch x -> Sealed Patch which confused me. Maybe a little notational tweak might help lead readers a bit, something like Sealed :: !(px C(,y)) -> Sealed p > +unseal :: Sealed a -> (FORALL(x) a C(x,) -> b) -> b > +unseal (Sealed a) f = f a Similarly, unseal :: Sealed px -> (FORALL(y) px C(,y) -> b) -> b -- Eric Kow http://www.loria.fr/~kow PGP Key ID: 08AC04F9 Merci de corriger mon français.
pgpvEdlqYfNzB.pgp
Description: PGP signature
_______________________________________________ darcs-devel mailing list [email protected] http://lists.osuosl.org/mailman/listinfo/darcs-devel
