[darcs-devel] darcs patch: Replace colour by color to uniformise a ... (and 7 more)

David Roundy droundy at darcs.net
Wed Apr 23 11:22:33 UTC 2008


Thanks for the resend!

I have a few issues with some of these patches (see below).

On Tue, Apr 22, 2008 at 03:59:40PM +0000, Nicolas Pouillard wrote:
> Wed Apr 16 09:47:42 CEST 2008  Nicolas Pouillard <nicolas.pouillard at gmail.com>
>   * Replace colour by color to uniformise a bit.
> 
> Wed Apr 16 09:59:54 CEST 2008  Nicolas Pouillard <nicolas.pouillard at gmail.com>
>   * Define unDoc as field of Doc.
> 
> Sun Apr 20 14:21:43 CEST 2008  Nicolas Pouillard <nicolas.pouillard at gmail.com>
>   * Little style change.

Up to here it looks good...

> Sun Apr 20 14:25:00 CEST 2008  Nicolas Pouillard <nicolas.pouillard at gmail.com>
>   * Refactor a little the color handling.

Here you've reversed a few optimizations that may not be terribly
important, but I'm not sure that you intended to do so, and there doesn't
seem a good reason to undo these optimizations.

> Sun Apr 20 14:28:11 CEST 2008  Nicolas Pouillard <nicolas.pouillard at gmail.com>
>   * Add two colors (cyan and magenta), but not use them yet.
> 
> Sun Apr 20 15:52:08 CEST 2008  Nicolas Pouillard <nicolas.pouillard at gmail.com>
>   * Export Printer.(<?>).
> 
> Sun Apr 20 15:52:38 CEST 2008  Nicolas Pouillard <nicolas.pouillard at gmail.com>
>   * Add line coloring support in Printer and ColourPrinter.

I'm not at all clear what the purpose of "line" coloring is here.  It seems
that we could more easily have "Doc" coloring that has the same effect with
greater efficiency.  i.e. why not just make a coloring function that is
roughly (set_color c <> d <> reset_color)? I don't see what we gain by
setting and resetting the color on every line.

> Sun Apr 20 15:52:52 CEST 2008  Nicolas Pouillard <nicolas.pouillard at gmail.com>
>   * Use the lineColoring to prettify hunks with colors.


>  import Printer (Printer, Printers, Printers'(..), Printable(..), Color(..),
> -                invisiblePrinter, (<>), Doc(Doc,unDoc), unsafeBoth, simplePrinter, hcat,
> +                invisiblePrinter, (<>), Doc(Doc,unDoc), unsafeBothText, simplePrinter, hcat,
>                  unsafeText, unsafeChar, space, unsafePackedString,
>                  renderStringWith )

You eliminate here unsafeBoth in favor of a new unsafeBothText, which seems
very odd, as the only reason for unsafeBoth was an efficiency gain (at the
expense of extra memory use), which you negate.  As a result, you may as
well use unsafeText, which is just as efficient as your code, and would
simplify the code by removing one function.

>  dollar, cr :: Doc
> -dollar = unsafeBoth  "$" (packString  "$")
> -cr     = unsafeBoth "\r" (packString "\r")
> +dollar = unsafeBothText "$"
> +cr     = unsafeBothText "\r"

Actually, for dollar and cr, your unsafeBothText is in fact as efficient
(and easier to use) than unsafeBoth.

> -make_color :: Color -> Doc -> Doc
> -make_color Blue  = make_blue
> -make_color Red   = make_red
> -make_color Green = make_green
> +make_color, make_color' :: Color -> Doc -> Doc

Here, a stylistic comment:  I prefer to not use combined type declarations
like this, but instead to keep the type declaration together with the
function definition.  There it serves as useful documentation, right where
you can easily find it when you're trying to figure out what the function
is doing (which can be tricky with these Docs).

> hunk ./src/Darcs/ColourPrinter.lhs 209
> -make_asciiart :: Doc -> Doc
> -make_asciiart x = unsafeBoth "[_" (packString "[_")
> -               <> x
> -               <> unsafeBoth "_]" (packString "_]")
> +make_color' = with_color . set_color
>  
> hunk ./src/Darcs/ColourPrinter.lhs 211
> -make_bold :: Doc -> Doc
> -make_bold x = unsafeBoth "\x1B[01m" (packString "\x1B[01m")
> -              <> x
> -              <> reset_color
> +-- memoized version of make_color'
> +make_color Blue    = make_color' Blue
> +make_color Red     = make_color' Red
> +make_color Green   = make_color' Green

I don't see how this achieves any memoization... and perhaps that's the
reason why your code seems to be a regression.  Can you explain how this
occurs?

> hunk ./src/Darcs/ColourPrinter.lhs 216
> -make_invert :: Doc -> Doc
> -make_invert x = unsafeBoth "\x1B[07m" (packString "\x1B[07m")
> -                <> x
> -                <> reset_color
> +set_color :: Color -> String
> +set_color Blue    = "\x1B[01;34m" -- bold blue
> +set_color Red     = "\x1B[01;31m" -- bold red
> +set_color Green   = "\x1B[01;32m" -- bold green
>  
> hunk ./src/Darcs/ColourPrinter.lhs 221
> -make_blue :: Doc -> Doc
> -make_blue x = unsafeBoth "\x1B[01;34m" (packString "\x1B[01;34m")
> -           <> x
> -           <> reset_color

I guess it looks like the old code was only half-better, in that
reset_color was computed only once, but the "blue" string got packed once
for each blue bit of text.

> +make_asciiart :: Doc -> Doc
> +make_asciiart x = unsafeBothText "[_" <> x <> unsafeBothText "_]"
>  
> hunk ./src/Darcs/ColourPrinter.lhs 224
> -make_red :: Doc -> Doc
> -make_red x = unsafeBoth "\x1B[01;31m" (packString "\x1B[01;31m")
> -          <> x
> -          <> reset_color
> +reset_color :: String
> +reset_color = "\x1B[00m"
>  
> hunk ./src/Darcs/ColourPrinter.lhs 227
> -make_green :: Doc -> Doc
> -make_green x = unsafeBoth "\x1B[01;32m" (packString "\x1B[01;32m")
> -            <> x
> -            <> reset_color
> +with_color :: String -> Doc -> Doc
> +with_color c =
> +   let c' = unsafeBothText c
> +       r' = unsafeBothText reset_color
> +   in \x -> c' <> x <> r'

Here's where it seems like we will keep running unsafeBothText (which calls
packString, which can be slow) twice for every color we use.  With just a
bit of tedium, we could pretty easily store these PackedStrings, and the
coloring would be more efficient.  Maybe this doesn't matter at all.

> hunk ./src/Darcs/ColourPrinter.lhs 233
> -reset_color :: Doc
> -reset_color = unsafeBoth "\x1B[00m" (packString "\x1B[00m")

Here, for instance, was where the reset_color was memoized.  Why did you
change it to be a String? And why not define all these guys as Docs
instead? If we don't memoize, then unsafeText is as efficient as
unsafeBothText, and its code is much simpler.

> [Export Printer.(<?>).
> Nicolas Pouillard <nicolas.pouillard at gmail.com>**20080420135208] hunk ./src/Darcs/ColourPrinter.lhs 9
>  import System.IO ( stderr )
>  import Darcs.External (getTermNColors)
>  import Printer (Printer, Printers, Printers'(..), Printable(..), Color(..),
> -                invisiblePrinter, (<>), Doc(Doc,unDoc), unsafeBothText, simplePrinter, hcat,
> +                invisiblePrinter, (<>), (<?>), Doc(Doc,unDoc), unsafeBothText, simplePrinter, hcat,
>                  unsafeText, unsafeChar, space, unsafePackedString,
>                  renderStringWith )
>  import Data.Char ( isAscii, isPrint, isSpace, isControl, ord, chr, intToDigit )

I'd prefer not to export <?>, if we can avoid it.  Every weird combinator
that we use makes the code a bit harder to read, and this seems like one we
can pretty well do without.

> [Add line coloring support in Printer and ColourPrinter.
> Nicolas Pouillard <nicolas.pouillard at gmail.com>**20080420135238]

Here I'm not sure why this needs to be "line" coloring, as I mention above.
It seems simpler to just color a whole block of text--which means we
wouldn't need to use prefix, and you wouldn't need to add the lineColorS
suffix.  The only reason for this that I can imagine is if for some reason
the color is automatically reset at the end of each line, but I can't see
why that would happen...
-- 
David Roundy
Department of Physics
Oregon State University


More information about the darcs-devel mailing list