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

[Peter Wang]

On Tue, 12 Feb 2013 19:33:28 +1100, Peter Moulder <peter.moulder at monash.edu> wrote:
> 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.

The problem was passing #code_points as the end parameter to
string.foldl_between instead of #code_units.
Since #code_points <= #code_units it could stop processing before the
actual end of the string, so char.digit_to_int is never called for the
non-digit code point.

> 
> (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 ?

Whatever it had accumulated up to, e.g. string.to_int("-123Σ", -123)

> 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.

The buggy tests would have failed had they been in the test suite.

> 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.)

We don't have that sort of infrastructure, but we could make it a
convention to add the broken tests *prior* to the fix in a patch series.

Peter