[m-rev.] diff: fix string.base_string_to_int succeeding incorrectly

Peter Moulder peter.moulder at monash.edu
Tue Feb 12 19:33:28 AEDT 2013


On Mon, Feb 11, 2013 at 11:09:11AM +1100, Peter Wang wrote:

> Fix string.base_string_to_int succeeding incorrectly.
> 
> string.base_string_to_int would incorrectly succeed on an input string
> containing a code point of multiple code units.

Are you sure?  I don't have an up-to-date checkout (so e.g. I haven't checked
whether count_codepoints is doing something I wouldn't expect), but from what I
can see, char.digit_to_int would fail even without applying this patch, for all
strings where the first K code points use more than K code units.

(I tried to get an up-to-date checkout, but I'm getting Connection timed out.
Might be a network/firewall problem at my end.)

If one of the tests added by this diff succeeded before applying this patch,
then what was the resulting Int value ?

If none of the tests added by this diff succeeded before applying this patch,
then that seems like a weakness in the testing system, because it suggests that
buggy tests (that pass when they weren't intended to pass) would give a false
sense of confidence in new code being checked in.  Some projects have tooling
to ensure that a purported fix is accompanied by at least one test case that
fails without the fix and succeeds with the fix.  (I.e. the tool first tries
applying just the test-suite part of the patch and runs at least the new tests
and checks that at least one fails, before applying the rest of the patch and
checking that at least the new tests all succeed.  There's room for variation
in how failures in existing tests are handled.)


(Regardless, the diff does look like an improvement.)

pjrm.



More information about the reviews mailing list