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 <[email protected]> > >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 _______________________________________________ LibreOffice mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/libreoffice
