[darcs-devel] darcs patch: rewrite ColourPrinter with a policy data... (and 17 more)

[Tommy Pettersson]

On Fri, Apr 08, 2005 at 07:41:50AM -0400, David Roundy wrote:
> > hunk ./ColourPrinter.lhs 76
> >                     then  \c -> color po c . escapePrinter po{poColor=False}
> >                     else  \_ -> escapePrinter po
> >  
> > +userchunkPrinter :: Policy -> Printer
> > +userchunkPrinter po p
> > + | not (poEscape po)   = simplePrinter p
> > + | not (poTrailing po) = escapePrinter po p
> > + | otherwise           = pr p
> > + where
> > +  pr (S s)      = prString s
> > +  pr (Both s _) = prString s
> > +  pr (PS ps)    = prPS ps
> 
> In the case of Both, I think it would be more efficient to go with prPS on
> the packed version, rather than using the String version.  True, you end up

Ah, that would of course be better.


> > +  prPS ps = let haveTrailingSpc = (not.nullPS) $ takeWhilePS isSpace (reversePS ps)
> > +            in if haveTrailingSpc
> > +                then prString (unpackPS ps)
> > +                else escapePrinter po p
> 
> I don't like the fact that reversePS has to make a separate copy of the
> string here, just to look for trailing whitespace.  Note that takeWhilePS
> (and similar functions) don't copy any of the actual data--just copy the
> pointer and introduce a new offset and length.  So I'd rather introduce a
> takeWhileFromEndPS which could do this more efficiently.


I don't know what I was thinking when I wrote this anyway.
It would be simpler and just as clear with:

  if (isSpace.lastPS) ps


I'll send patches with fixes for these issues later.
The latter one can likely be combined with the trailing ^M
special casing.



-- 
Tommy Pettersson <ptp at lysator.liu.se>