Re: svn commit: r986453 - /subversion/branches/performance/subversion/libsvn_subr/subst.c

[Julian Foad <ju...@wandisco.com>]

On Thu, 2010-11-04, Stefan Fuhrmann wrote:
> On 30.10.2010 20:30, Daniel Shahaf wrote:
> > stefan2@apache.org wrote on Tue, Aug 17, 2010 at 19:04:21 -0000:
[...]
> >> +               /* Found an interesting char or EOF in the next 4 bytes.
> >> +                  Find its exact position. */
> >> +               while ((p + len)<  end&&  !interesting[(unsigned char)p[len]])
> >> +                 ++len;
> >> +            }
> >> +          while (b->nl_translation_skippable&&    /* can skip EOLs at all */
> >> +                 p + len + 2<  end&&              /* not too close to EOF */
> >> +                 eol_unchanged (b, p + len));     /* EOL format already ok */
> >>
> > Haven't read the whole hunk yet, but this jumped to my eye:
> >
> > Since nl_translation_skippable is an svn_tristate_t, shouldn't this test
> > explicitly for 'svn_tristate_true'?  (Otherwise, the test would evaluate
> > to true even for svn_tristate_false...)
> >
> > Perhaps we should name the variable in a way that indicates its
> > non-booleanness --- e.g., tri_nl_translation_skippable --- ?

> Fixed in 1029335. But modifying the tristate enum
> definition as proposed in a recent post would have
> fixed this as well.

Modifying the tristate enum (so _false==FALSE and _true==TRUE) would
only have fixed this if the definition of fixed is "==_true or
==_unknown".  That's not the same as "==_true" which you chose in
r1029335.

- Julian