> -----Original Message-----
> From: Julian Foad [mailto:julianfoad@btopenworld.com]
>
> > if (b->keyword_off > SVN_KEYWORD_MAX_LEN ||
>
> Should that be ">="? By the time it's greater, we'll already
> have written at least one character off the end of the buffer.
I don't think so. b-keyword_off was post-incremented in the previous line,
so we can't write off the end of the buffer.
> Check what's going to happen at the boundary condition: will
> it do the right thing for a valid keyword-pattern of exactly
> MAX_LEN? MAX_LEN-1? MAX_LEN+1?
> What about a non-keyword pattern of those lengths?
I've added lines 76-82 in translate-test.c for testing for these border
cases. They all seem to work as expected.
> I think "b->keyword_off >= SVN_KEYWORD_MAX_LEN" will need to
> go after "translate_keyword()", but see what you think.
No problem with that.
However, I don't see a change in behaviour when I implement the >= change
nor the line-order change. The new tests should have captured this change. I
guess that b->keyword_off == SVN_KEYWORD_MAX_LEN will not be reached there
because it's already handled earlier in the else block at line 998.
>
> > keyword_matches == FALSE ||
> > translate_keyword (b->keyword_buf,
> &b->keyword_off,
> > keyword_name,
> b->expand, b->keywords))
> > {
>
>
> Since I've done some tidying while studying it, I attach my
> version of the patch and also a diff from yours to mine. The
> changes are mostly issues of style and comments. The
> significant changes are: use "apr_size_t" for
> "next_sign_off", because that is the type used for other
> values to which it relates (b->keyword_off); don't initialise
> the new flag at its definition because it is initialised at
> its first use; add a missing comma in the array of test lines.
>
> If you think the two things above (changing the limit test to
> ">=" and moving it to the end of the condition) are the only
> further changes needed, I can do that and commit it for you,
> otherwise I look forward to seeing the next version.
The attached patch is based on yours and includes these two changes and some
extra tests, no new changes in the algorithm itself.
> - Julian
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Feb 14 22:25:59 2006