On Thu, 2010-11-04, Stefan Fuhrmann wrote: > On 30.10.2010 20:30, Daniel Shahaf wrote: > > [email protected] 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
