Re: Obsoleting RTL_CONSTASCII_USTRINGPARAM

On Thursday 09 of February 2012, Caolán McNamara wrote:
> On Thu, 2012-02-09 at 18:16 +0100, Lubos Lunak wrote:
> > suggests that OString in fact does not stop at \0's when handling
> > strings. I'm kind of baffled by this, I personally consider it a flaw
> > that a string class does this, but the unittest is very explicit about
> > this. Does somebody know?
>
> svtools/source/svrtf/parrtf.cxx:345 is a in-action example. Where rtf
> can describe a character in hex bytes and for list strings '00 to '09
> are used as placeholders for the list level to substitute into the
> string, or something like that. Vaguely recall there might be similar
> stuff in some of the binary ms file formats, where their strings tend to
> be pascal-style length-prefixed strings and while embedded nulls aren't
> common they might arise a bit. Anyway, as it stands embedded NULLS are
> legal in O[U]Strings

 I think using something like vector here would be a better choice (or rather 
not a bad choice), but ok, then I can either move the check to the new ctor 
or assume that the const char[][N] case is so unlikely to happen that it's 
not worth checking for explicitly. I guess I'll just go with the latter.

> > So while I can be talked into adding the explicit, I can't find any
> > good reason for doing that.
>
> core/sal/textenc/textenc.cxx: In constructor
> ‘{anonymous}::FullTextEncodingData::FullTextEncodingData()’:
> core/sal/textenc/textenc.cxx:397:68: error: ISO C++ says that these are
> ambiguous, even though the worst conversion for the first is better than
> the worst conversion for the second: [-Werror]
> core/solver/unxlngi6/inc/osl/module.hxx:156:24: note: candidate 1: void
> (* osl::Module::getFunctionSymbol(const char*) const)()
> core/solver/unxlngi6/inc/osl/module.hxx:150:24: note: candidate 2: void
> (* osl::Module::getFunctionSymbol(const rtl::OUString&))()
>
> if I stick in explicit the above goes away.

 Fortunately it also goes away if the osl::Module class gets fixed. The 
original developer of the class was sloppy with constness, whereas whoever 
added the recent OUString overload did it properly, so the ambiguity stems 
from having to chose either doing const conversion for the 'this' parameter 
or literal->OUString conversion for the normal parameter.

 But yes, this shows that this cannot be done completely carelessly. I've now 
checked a complete rebuild and there were some more cases of ambiguities, but 
only a handful of them, mostly OString vs OUString overloads. Those cases can 
be fixed by an extra overload, removing needless overload, explicit 
conversion and in some cases it even looks like the API is in fact broken 
(dbaui::OGenericUnoController::openHelpAgent() does something different for 
the overloads, INetURLObject decodes their contents differently). Still, I 
think the benefits outweight these few cases.

> I had wondered if
> http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#757 would
> make a difference here, but locally seems to work fine in my gcc
> with/without c++11x support

 I checked this before starting, as far as I understand it, this is triggered 
by what type is used as the template argument, but in this case the template 
operates on const char[N] and the template argument is a simple numeric 
value, so the SAL_N_ELEMENTS limitation does not apply here.

 Updated patch attached.

-- 
 Lubos Lunak
 l.lunak@suse.cz

diff --git a/basic/source/uno/namecont.cxx b/basic/source/uno/namecont.cxx
index db0bf66..db72a4f 100644
--- a/basic/source/uno/namecont.cxx
+++ b/basic/source/uno/namecont.cxx
@@ -1001,13 +1001,13 @@ sal_Bool SfxLibraryContainer::init_Impl(
         INetURLObject aUserBasicInetObj( String(maLibraryPath).GetToken(1) );
         OUString aStandardStr( RTL_CONSTASCII_USTRINGPARAM("Standard") );
 
-        static char strPrevFolderName_1[] = "__basic_80";
-        static char strPrevFolderName_2[] = "__basic_80_2";
+        static char const strPrevFolderName_1[] = "__basic_80";
+        static char const strPrevFolderName_2[] = "__basic_80_2";
         INetURLObject aPrevUserBasicInetObj_1( aUserBasicInetObj );
         aPrevUserBasicInetObj_1.removeSegment();
         INetURLObject aPrevUserBasicInetObj_2 = aPrevUserBasicInetObj_1;
-        aPrevUserBasicInetObj_1.Append( strPrevFolderName_1 );
-        aPrevUserBasicInetObj_2.Append( strPrevFolderName_2 );
+        aPrevUserBasicInetObj_1.Append( rtl::OString( strPrevFolderName_1 ));
+        aPrevUserBasicInetObj_2.Append( rtl::OString( strPrevFolderName_2 ));
 
         // #i93163
         bool bCleanUp = false;
@@ -1072,7 +1072,7 @@ sal_Bool SfxLibraryContainer::init_Impl(
                 String aFolderUserBasic = aUserBasicInetObj.GetMainURL( INetURLObject::NO_DECODE );
                 INetURLObject aUserBasicTmpInetObj( aUserBasicInetObj );
                 aUserBasicTmpInetObj.removeSegment();
-                aUserBasicTmpInetObj.Append( "__basic_tmp" );
+                aUserBasicTmpInetObj.Append( rtl::OString( "__basic_tmp" ));
                 String aFolderTmp = aUserBasicTmpInetObj.GetMainURL( INetURLObject::NO_DECODE );
 
                 mxSFI->move( aFolderUserBasic, aFolderTmp );
@@ -1195,10 +1195,10 @@ sal_Bool SfxLibraryContainer::init_Impl(
         {
             SAL_WARN("basic", "Upgrade of Basic installation failed somehow");
 
-            static char strErrorSavFolderName[] = "__basic_80_err";
+            static const char strErrorSavFolderName[] = "__basic_80_err";
             INetURLObject aPrevUserBasicInetObj_Err( aUserBasicInetObj );
             aPrevUserBasicInetObj_Err.removeSegment();
-            aPrevUserBasicInetObj_Err.Append( strErrorSavFolderName );
+            aPrevUserBasicInetObj_Err.Append( rtl::OString( strErrorSavFolderName ));
             String aPrevFolder_Err = aPrevUserBasicInetObj_Err.GetMainURL( 
INetURLObject::NO_DECODE );
 
             bool bSaved = false;
diff --git a/dbaccess/source/ui/browser/unodatbr.cxx b/dbaccess/source/ui/browser/unodatbr.cxx
index 6e31141..705cb24 100644
--- a/dbaccess/source/ui/browser/unodatbr.cxx
+++ b/dbaccess/source/ui/browser/unodatbr.cxx
@@ -1916,7 +1916,7 @@ void SbaTableQueryBrowser::Execute(sal_uInt16 nId, const Sequence< 
PropertyValue
             break;
 
         case ID_TREE_CLOSE_CONN:
-            openHelpAgent( HID_DSBROWSER_DISCONNECTING );
+            openHelpAgent( rtl::OString( HID_DSBROWSER_DISCONNECTING ));
             closeConnection( m_pTreeView->getListBox().GetRootLevelParent( 
m_pTreeView->getListBox().GetCurEntry() ) );
             break;
 
diff --git a/desktop/source/deployment/inc/dp_misc.h b/desktop/source/deployment/inc/dp_misc.h
index 9e91253..29fd140 100644
--- a/desktop/source/deployment/inc/dp_misc.h
+++ b/desktop/source/deployment/inc/dp_misc.h
@@ -128,15 +128,6 @@ oslProcess raiseProcess( ::rtl::OUString const & appURL,
 DESKTOP_DEPLOYMENTMISC_DLLPUBLIC
 void writeConsole(::rtl::OUString const & sText);
 
-/** writes the argument string to the console.
-    On Linux/Unix/etc. the string is passed into fprintf without any conversion.
-    On Windows the string is converted to UTF16 assuming the argument is UTF8
-    encoded. The UTF16 string is written to stdout with WriteFile. unopkg.com
-    reads the data and prints them out using WriteConsoleW.
-*/
-DESKTOP_DEPLOYMENTMISC_DLLPUBLIC
-void writeConsole(::rtl::OString const & sText);
-
 /** writes the argument to the console using the error stream.
     Otherwise the same as writeConsole.
 */
@@ -144,13 +135,6 @@ DESKTOP_DEPLOYMENTMISC_DLLPUBLIC
 void writeConsoleError(::rtl::OUString const & sText);
 
 
-/** writes the argument to the console using the error stream.
-    Otherwise the same as writeConsole.
-*/
-DESKTOP_DEPLOYMENTMISC_DLLPUBLIC
-void writeConsoleError(::rtl::OString const & sText);
-
-
 /** reads from the console.
     On Linux/Unix/etc. it uses fgets to read char values and converts them to OUString
     using osl_getThreadTextEncoding as target encoding. The returned string has a maximum
@@ -165,8 +149,6 @@ DESKTOP_DEPLOYMENTMISC_DLLPUBLIC
 */
 DESKTOP_DEPLOYMENTMISC_DLLPUBLIC
 void TRACE(::rtl::OUString const & sText);
-DESKTOP_DEPLOYMENTMISC_DLLPUBLIC
-void TRACE(::rtl::OString const & sText);
 
 /** registers or revokes shared or bundled extensions which have been
     recently added or removed.
diff --git a/desktop/source/deployment/registry/configuration/dp_configuration.cxx 
b/desktop/source/deployment/registry/configuration/dp_configuration.cxx
index 12e5f50..dd1b00b 100644
--- a/desktop/source/deployment/registry/configuration/dp_configuration.cxx
+++ b/desktop/source/deployment/registry/configuration/dp_configuration.cxx
@@ -244,8 +244,8 @@ BackendImpl::BackendImpl(
             }
             catch (const Exception &e)
             {
-                rtl::OStringBuffer aStr( "Exception loading legacy package database: '" );
-                aStr.append( rtl::OUStringToOString( e.Message, osl_getThreadTextEncoding() ) );
+                rtl::OUStringBuffer aStr( "Exception loading legacy package database: '" );
+                aStr.append( e.Message );
                 aStr.append( "' - ignoring file, please remove it.\n" );
                 dp_misc::writeConsole( aStr.getStr() );
             }
diff --git a/sal/CppunitTest_sal_rtl_strings.mk b/sal/CppunitTest_sal_rtl_strings.mk
index fe6da66..c1db17d 100644
--- a/sal/CppunitTest_sal_rtl_strings.mk
+++ b/sal/CppunitTest_sal_rtl_strings.mk
@@ -32,6 +32,7 @@ $(eval $(call gb_CppunitTest_add_exception_objects,sal_rtl_strings,\
     sal/qa/rtl/strings/test_oustring_convert \
     sal/qa/rtl/strings/test_oustring_endswith \
     sal/qa/rtl/strings/test_oustring_noadditional \
+    sal/qa/rtl/strings/test_oustring_stringliterals \
 ))
 
 $(eval $(call gb_CppunitTest_add_linked_libs,sal_rtl_strings,\
diff --git a/sal/inc/osl/module.hxx b/sal/inc/osl/module.hxx
index c8d39a2..a7f9c35 100644
--- a/sal/inc/osl/module.hxx
+++ b/sal/inc/osl/module.hxx
@@ -152,11 +152,28 @@ public:
         return ( osl_getFunctionSymbol( m_Module, ustrFunctionSymbolName.pData ) );
     }
 
+    /** 
+     * @overload
+     * @since LibreOffice 3.6
+     */
+    oslGenericFunction SAL_CALL getFunctionSymbol( const ::rtl::OUString& ustrFunctionSymbolName ) 
const
+    {
+        return ( osl_getFunctionSymbol( m_Module, ustrFunctionSymbolName.pData ) );
+    }
+
     /// @since LibreOffice 3.5
     oslGenericFunction SAL_CALL getFunctionSymbol(char const * name) const {
         return osl_getAsciiFunctionSymbol(m_Module, name);
     }
 
+    /** 
+     * @overload
+     * @since LibreOffice 3.6
+     */
+    oslGenericFunction SAL_CALL getFunctionSymbol(char const * name) {
+        return osl_getAsciiFunctionSymbol(m_Module, name);
+    }
+
     operator oslModule() const
     {
         return m_Module;
diff --git a/sal/inc/rtl/ustring.hxx b/sal/inc/rtl/ustring.hxx
index ace4b1a..d3f4fde 100644
--- a/sal/inc/rtl/ustring.hxx
+++ b/sal/inc/rtl/ustring.hxx
@@ -168,6 +168,65 @@ public:
     }
 
     /**
+      New string from an 8-Bit string literal that contains only characters
+      in the ASCII character set. All string literals in the codebase are
+      assumed to be only ASCII, so this constructor allows an efficient
+      and convenient way to create OUString instances from literals.
+
+      @param    value           the 8-bit string literal
+
+      @exception std::bad_alloc is thrown if an out-of-memory condition occurs
+    */
+    template< int N >
+    OUString( const char (&literal)[ N ] )
+    {
+        pData = 0;
+        rtl_string2UString( &pData, literal, N - 1, RTL_TEXTENCODING_ASCII_US, 0 );
+        if (pData == 0) {
+#if defined EXCEPTIONS_OFF
+            SAL_WARN("sal", "std::bad_alloc but EXCEPTIONS_OFF");
+#else
+            throw std::bad_alloc();
+#endif
+        }
+    }
+
+    /**
+     * This overload exists only to avoid creating instances directly from (non-const) char[],
+     * which would otherwise be picked up by the optimized const char[] constructor.
+     * Since the non-const array cannot be guaranteed to contain only ASCII characters,
+     * this needs to be prevented.
+     * 
+     * It is an error to try to call this overload.
+     *
+     * @internal
+     */
+    template< int N >
+    OUString( char (&value)[ N ] )
+#ifndef RTL_STRING_UNITTEST
+        ; // intentionally not implemented
+#else
+    {
+        (void) value; // unused
+        pData = 0; // for the unittest create an empty string
+        rtl_uString_new( &pData );
+    }
+#endif
+
+#ifdef RTL_STRING_UNITTEST
+    /**
+     * Only used by unittests to detect incorrect conversions.
+     * @internal
+     */
+    template< typename T >
+    OUString( T )
+    {
+        pData = 0;
+        rtl_uString_new( &pData );
+    }
+#endif
+
+    /**
       New string from a 8-Bit character buffer array.
 
       @param    value           a 8-Bit character array.
@@ -1492,6 +1551,9 @@ public:
       values are not converted in any way. The caller has to make sure that
       all ASCII characters are in the allowed range between 0 and
       127. The ASCII string must be NULL-terminated.
+      
+      Note that for string literals it is simpler and more efficient
+      to directly use the OUString constructor.
 
       @param    value       the 8-Bit ASCII character string
       @return   a string with the string representation of the argument.
diff --git a/sal/inc/sal/log-areas.dox b/sal/inc/sal/log-areas.dox
index d89bbdc..7c2d8ce 100644
--- a/sal/inc/sal/log-areas.dox
+++ b/sal/inc/sal/log-areas.dox
@@ -25,6 +25,10 @@ certain functionality.
 
 @li oox.xmlstream - XmlStream class
 
+@section SAL/RTL
+
+@li rtl.string - rtl::OString, rtl::OUString and related functionality
+
 @section VCL
 
 @li vcl.gdi - the GDI part of VCL: devices, bitmaps, etc.
diff --git a/sal/qa/rtl/strings/test_oustring_stringliterals.cxx 
b/sal/qa/rtl/strings/test_oustring_stringliterals.cxx
new file mode 100644
index 0000000..142f53d
--- /dev/null
+++ b/sal/qa/rtl/strings/test_oustring_stringliterals.cxx
@@ -0,0 +1,91 @@
+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
+/*************************************************************************
+ *
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * Copyright 2000, 2010 Oracle and/or its affiliates.
+ *
+ * OpenOffice.org - a multi-platform office productivity suite
+ *
+ * This file is part of OpenOffice.org.
+ *
+ * OpenOffice.org is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License version 3
+ * only, as published by the Free Software Foundation.
+ *
+ * OpenOffice.org is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License version 3 for more details
+ * (a copy is included in the LICENSE file that accompanied this code).
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * version 3 along with OpenOffice.org.  If not, see
+ * <http://www.openoffice.org/license.html>
+ * for a copy of the LGPLv3 License.
+ *
+ ************************************************************************/
+
+// activate the extra needed ctor
+#define RTL_STRING_UNITTEST
+
+#include "sal/config.h"
+#include "sal/precppunit.hxx"
+
+#include <cppunit/TestFixture.h>
+#include <cppunit/extensions/HelperMacros.h>
+#include "rtl/string.h"
+#include "rtl/ustring.hxx"
+
+namespace test { namespace oustring {
+
+class StringLiterals: public CppUnit::TestFixture
+{
+private:
+    void checkCtors();
+
+    void testcall( const char str[] );
+    // invalid conversions will trigger templated OUString ctor that creates an empty string
+    // (see RTL_STRING_UNITTEST)
+    bool validConversion( const rtl::OUString& str ) { return !str.isEmpty(); }
+
+CPPUNIT_TEST_SUITE(StringLiterals);
+CPPUNIT_TEST(checkCtors);
+CPPUNIT_TEST_SUITE_END();
+};
+
+void test::oustring::StringLiterals::checkCtors()
+{
+    CPPUNIT_ASSERT( validConversion( rtl::OUString( "test" )));
+    const char good1[] = "test";
+    CPPUNIT_ASSERT( validConversion( rtl::OUString( good1 )));
+
+    CPPUNIT_ASSERT( !validConversion( rtl::OUString( (const char*) "test" )));
+    const char* bad1 = good1;
+    CPPUNIT_ASSERT( !validConversion( rtl::OUString( bad1 )));
+    char bad2[] = "test";
+    CPPUNIT_ASSERT( !validConversion( rtl::OUString( bad2 )));
+    char* bad3 = bad2;
+    CPPUNIT_ASSERT( !validConversion( rtl::OUString( bad3 )));
+    const char* bad4[] = { "test1" };
+    CPPUNIT_ASSERT( !validConversion( rtl::OUString( bad4[ 0 ] )));
+    testcall( good1 );
+
+// This one is technically broken, since the first element is 6 characters test\0\0,
+// but there does not appear a way to detect this by compile time (runtime will complain).
+// RTL_CONSTASCII_USTRINGPARAM() has the same flaw.
+    const char bad5[][ 6 ] = { "test", "test2" };
+//    CPPUNIT_ASSERT( validConversion( rtl::OUString( bad5[ 0 ] )));
+    CPPUNIT_ASSERT( validConversion( rtl::OUString( bad5[ 1 ] )));
+}
+
+void test::oustring::StringLiterals::testcall( const char str[] )
+{
+    CPPUNIT_ASSERT( !validConversion( rtl::OUString( str )));
+}
+
+}} // namespace
+
+CPPUNIT_TEST_SUITE_REGISTRATION(test::oustring::StringLiterals);
+
+/* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/sal/rtl/source/ustring.cxx b/sal/rtl/source/ustring.cxx
index a37353c..38a0f69 100644
--- a/sal/rtl/source/ustring.cxx
+++ b/sal/rtl/source/ustring.cxx
@@ -39,6 +39,7 @@
 
 #include <string.h>
 #include <sal/alloca.h>
+#include <sal/log.hxx>
 
 #include "hash.hxx"
 #include "strimp.hxx"
@@ -594,7 +595,7 @@ static void rtl_string2UString_status( rtl_uString** ppThis,
             do
             {
                 /* Check ASCII range */
-                OSL_ENSURE( ((unsigned char)*pStr) <= 127,
+                SAL_WARN_IF( ((unsigned char)*pStr) > 127, "rtl.string",
                             "rtl_string2UString_status() - Found char > 127 and 
RTL_TEXTENCODING_ASCII_US is specified" );
 
                 *pBuffer = *pStr;
diff --git a/svl/source/misc/inethist.cxx b/svl/source/misc/inethist.cxx
index 16745c0..e9a2bb0 100644
--- a/svl/source/misc/inethist.cxx
+++ b/svl/source/misc/inethist.cxx
@@ -443,14 +443,14 @@ void INetURLHistory::NormalizeUrl_Impl (INetURLObject &rUrl)
             if (!rUrl.HasPort())
                 rUrl.SetPort (INETHIST_DEF_HTTP_PORT);
             if (!rUrl.HasURLPath())
-                rUrl.SetURLPath ("/");
+                rUrl.SetURLPath (rtl::OString("/"));
             break;
 
         case INET_PROT_HTTPS:
             if (!rUrl.HasPort())
                 rUrl.SetPort (INETHIST_DEF_HTTPS_PORT);
             if (!rUrl.HasURLPath())
-                rUrl.SetURLPath ("/");
+                rUrl.SetURLPath (rtl::OString("/"));
             break;
 
         default: