[darcs-devel] darcs patch: Replace colour by color to uniformise a ... (and 7 more)
Nicolas Pouillard
nicolas.pouillard at gmail.com
Wed Apr 23 12:08:10 UTC 2008
Excerpts from David Roundy's message of Wed Apr 23 13:22:33 +0200 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.
I've experienced stack-overflows while changing this code, and I now think that
I've finally not undone these optimizations, look at make_color and make_color'.
> > 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.
That was my first attempt, however terminal colors are a tricky thing, and
you can see it in your pager when you are seeing only a part of a hunk. If
you are breaking a hunk at the bottom of the screen you can see that the
'less' prompt is colored. If you are breaking a hunk at the top of the
screen, then you don't get color until the reaching the start.
> > 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.
Unless being wrong, the usage of 'Both' is still there with unsafeBothText.
> > 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.
Indeed if you share/name the result of unsafeBothText, you are as efficient.
> > -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).
Ok, that's a matter of style and I will try follow them in darcs.
> > 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?
Because something like "make_color' Blue" is a pure constant expression, it
will be then floated as constant, like in:
__make_color_Blue = make_color' Blue
...
make_color Blue = __make_color_Blue
...
I'm pretty sure to haven't lost the optimization, because loosing seems to
leads to stack-overflows.
> > 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.
Nop, since it's a constant expression GHC should lift it. That's the beauty
of being pure.
>
> > +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.
Yes each time that with_color is called, unsafeBothText is called... However
the use of with_color is managed with care in order to share this
computation.
>
> > 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.
You're right, but I needed the string without the pack and thought that
adding a reset_color_doc was not that useful since "with_color reset_color"
gets optimized.
> > [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.
However I can't avoid it, otherwise hunks that only add text gets an empty
line.
> > [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...
See my comment above. Moreover I've sent quite some time to polish these and
tried simpler things (and more complex :( ) before reaching that one.
Cheers,
--
Nicolas Pouillard aka Ertai
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 194 bytes
Desc: not available
Url : http://lists.osuosl.org/pipermail/darcs-devel/attachments/20080423/3b9038bc/attachment.pgp
More information about the darcs-devel
mailing list