[darcs-users] darcs patch: Add an explanation of headPermutationsFL... (and 6 more)
Reinier Lamers
tux_rocker at reinier.de
Wed Apr 22 21:32:27 UTC 2009
Hi,
Here's the review. I have some nitpicks on what I consider good style for
comments. I think that we should not allow questions in comments, and instead
try to find answers to such questions immediately. But I haven't read piles of
software project managements books, so maybe someone who has can correct me.
Regards,
Reinier
On Wednesday 22 April 2009 17:30:25 E.Y.Kow at brighton.ac.uk wrote:
>hunk ./src/Darcs/Patch/Permutations.hs 158
>+-- | 'headPermutationsFL' @p:>:ps@ returns all the permutations of the list
>+-- in which one element of @ps@ is commuted past @p@
>+--
>+-- Suppose we have a sequence of patches
>+--
>+-- > X h a y s-t-c k
>+--
>+-- Suppose furthermore that the patch @c@ depends on @t@, which in turn
>+-- depends on @s at . This function will return
>+--
>+-- > X :> h a y s t c k
>+-- > h :> X a y s t c k
>+-- > a :> X h y s t c k
>+-- > y :> X h a s t c k
>+-- > s :> X h a y t c k
>+-- > k :> X h a y s t c
Nice :-).
>+ -- | There is no difference between this and the 'commute' function
>+ -- other than the order. ('commutex' uses '(:<)' and 'commute'
>+ -- uses '(:>)'.
>+ --
>+ -- Question: if there is no difference, why do we have two different
>+ -- functions? Is it just convenience? Eric 2009-04-19
I believe that source code comments are not the right medium to ask questions.
If no current darcs hacker knows an answer to this, we should ask David.
>+-- | Patches whose concrete effect which can be expressed as a list of
>+-- primitive patches.
>+--
>+-- A minimal definition would be either of @effect@ or @effectRL at .
Nice, learned something there.
>+-- | 'addP' @x cy@ commutes @x@ past @cy@ if possible.
>+-- If it's not possible, it prepends @x@ co the context @c@
>+-- In either case, it returns the modified @cy at .
> addP :: (Patchy p, ToFromPrim p) => p C(x y) -> Non p C(y) -> Non p C(x)
> addP p n | Just n' <- p >* n = n'
> addP p (Non c x) = Non (p:>:c) x
This comment might be clearer about whether or not x is included in the result
(afaics it is in both cases). Eg. 'This function returns @Non xc y@ if @x@ and
@c@ do not commute, and returns @Non cx y@ otherwise'.
>hunk ./src/Darcs/Patch/Non.hs 100
>
>+-- | 'addPs' @xs cy@ commutes as many patches of @xs@ past @cy@ as
>+-- possible, stopping at the first patch that fails to commute.
>+-- Note the fact @xs@ is a 'RL'
>+--
>+-- Suppose we have
>+--
>+-- > x1 x2 x3 [c1 c2 y]
>+--
>+-- and that in our example @c1@ fails to commute past @x1@, this
>+-- function would commute down to
>+--
>+-- > x1 [c1'' c2'' y''] x2' x3'
>+--
>+-- and return @[x1 c1'' c2'' y'']@
Why are you using c1 and c2 here, instead of just a single c?
>[Haddock some simple functions in Darcs.Patch.Real.
>Eric Kow <kowey at darcs.net>**20090419151449
> Ignore-this: fd05c99677687056042dcee649dbe2b6
> The only thing in common in particular being that these are relatively
> straightforward to understand: is_duplicate, sealed2non and allNormals.
>] hunk ./src/Darcs/Patch/Real.hs 88
> Conflictor :: [Non RealPatch C(x)] -> FL Prim C(x y) -> Non RealPatch
> C(x) -> RealPatch C(y x) InvConflictor :: [Non RealPatch C(x)] -> FL Prim
> C(x y) -> Non RealPatch C(x) -> RealPatch C(x y)
>
>+-- | Either a 'Duplicate' or 'Etacilpud' patch
I'd prefer a full sentence here... "'is_duplicate' returns true if the only
argument is either a 'Duplicate' or 'Etacilpud' patch".
>+-- | If @xs@ consists only of 'Normal' patches, 'allNormal' @xs@ returns
>+-- @Just pxs@ those patches (so @lengthFL pxs == lengthFL xs@)
I'd expect the comment to also state what the function returns if the patches
are not all normal.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.osuosl.org/pipermail/darcs-users/attachments/20090422/0f13458d/attachment.pgp>
More information about the darcs-users
mailing list