Re: [Libreoffice-commits] core.git: use startsWith() instead of compareToAscii()

Thanks Stephan, good catch!

Short version: this was, and should remain, an equality test and not a
"string begins with, but can continue with anything" test.

On Mon, Mar 11, 2013 at 12:05:40PM +0100, Stephan Bergmann wrote:
> On 03/11/2013 11:10 AM, Thomas Arnhold wrote:
> > commit 937b63af3322f7f8b5e869b2c7431a2deaec3113
> > Author: Thomas Arnhold <thomas@arnhold.org>
> > Date:   Sat Mar 9 22:47:20 2013 +0100
> >
> >     use startsWith() instead of compareToAscii()
> >
> >     brain damage...
> >
> >     Change-Id: I4dc63c7346f724eded9ac7b82cda25c2bb60beff
> [...]
> > diff --git a/connectivity/source/drivers/hsqldb/HDriver.cxx 
> > b/connectivity/source/drivers/hsqldb/HDriver.cxx
> > index 3f2a96b..d58c8f5 100644
> > --- a/connectivity/source/drivers/hsqldb/HDriver.cxx
> > +++ b/connectivity/source/drivers/hsqldb/HDriver.cxx
> > @@ -399,7 +399,7 @@ namespace connectivity
> >      {
> >          sal_Bool bEnabled = sal_False;
> >          OSL_VERIFY_EQUALS( jfw_getEnabled( &bEnabled ), JFW_E_NONE, "error in jfw_getEnabled" 
> > );
> > -        return bEnabled  && 
> > url.compareToAscii("sdbc:embedded:hsqldb",sizeof("sdbc:embedded:hsqldb")) == 0;
> > +        return bEnabled  && url.startsWith("sdbc:embedded:hsqldb");

> Note that sizeof("...") == strlen("...") + 1, so the call to
> compareToAscii included the terminating NUL of the string literal in
> the comparison, so would never have returned zero (unless url
> contained embedded NUL characters, which is unlikely).

So,
 compareToAscii never returned zero
means
 url.compareToAscii(..) == 0 never was true
so you are saying that
 return bEnabled &&  url.compareToAscii("sdbc:embedded:hsqldb",sizeof("sdbc:embedded:hsqldb")) == 0
was in fact equivalent to
 return false;

In other words, you are saying that foo.compareToAscii(bar, n) is more or less a
memcmp(&foo, &bar, min(strlen(foo),n)); provided foo is long enough it
always compares *exactly* n characters/bytes, never less.

I *strongly* doubt that, since it would mean that embedded hsqldb .odb
files WOULD NOT WORK AT ALL.

Indeed, looking in sal/rtl/ustring.cxx at function
rtl_ustr_ascii_shortenedCompare_WithLength,
it seems to me it handles the terminating NULL specially:

    while ( (nShortenedLength > 0) &&
            (pStr1 < pStr1End) && *pStr2 )
    {
       ...
    }

    (...)

    if ( *pStr2 )
    {
       ...
    }
    else
    {
       nRet = pStr1End - pStr1;
    }

That is, it returns 0 if and only if its first argument (that is, in
our example, the value of the url variable) it precisely equal to the
pStr2 argument.

In other words, I think foo.compareToAscii(bar, n) is more or less a
 memcmp(&foo, &bar, min(strlen(foo),strlen(bar),n))
that is more or less a
 strncmp(&foo, &bar, n)
In other words, if bar is of length less than 1000, then all these
calls are equivalent:
 foo.compareToAscii(bar, sizeof(bar))
 foo.compareToAscii(bar, strlen(bar))
 foo.compareToAscii(bar, 1000)
 foo == bar

> Not sure about the details of that sdbc URL scheme, whether what one
> wants there is url.startsWith("sdbc:embedded:hsqldb") or
> url.startsWith("sdbc:embedded:hsqldb:") or
> url=="sdbc:embedded:hsqld"

It should be url=="sdbc:embedded:hsqldb". Most drivers have in their
acceptsURL a test of the kind url.startsWith("sdbc:foo:bar:") because
they expect extra information after the "sdbc:foo:bar"; in this
particular case of embedded HSQLDB, there is no extra information, and
the full exact URI is "sdbc:embedded:hsqldb".

A test like:
 url.startsWith("sdbc:embedded:hsqldb:")
would break all/older files since their sdbc URI is
"sdbc:embedded:hsqldb" and this would not match.

A test like:
 url.startsWith("sdbc:embedded:hsqldb")
would wrongly match a putative future URI "sdbc:embedded:hsqldb2",
which the current driver is *not* able to handle.

> (and, while we are at it, at least for the initial "sdbc" part, URI
> syntax would mandated case-insensitive comparison anyway, unless the
> given url is known to be normalized to lowercase),

I'm not sure if it is guaranteed normalised. If it is not, we have
this bug (of case-sensitivity) all over the place, not only in
embedded HSQLDB.

-- 
Lionel