[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