On Thu, 2010-11-04, Stefan Fuhrmann wrote:
> On 30.10.2010 20:30, Daniel Shahaf wrote:
> > stefan2_at_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
Received on 2010-11-04 01:11:49 CET