Introduce QBStr BSTR RAII wrapper

Use of base BSTR instances can be error prone and verbose. This patch
introduces a QBStr class that wraps the Microsoft BSTR string type.

Code that can be tested is updated to use the new QBStr instead of bare
BSTR instances.

Change-Id: If55e7d18426d1751357c9b4512abcfc4d10afbb4
Reviewed-by: Pavel Dubsky <pavel.dubsky@qt.io>
Reviewed-by: Oliver Wolff <oliver.wolff@qt.io>

9 files changed, 306 insertions, 106 deletions

diff --git a/src/activeqt/axbase/CMakeLists.txt b/src/activeqt/axbase/CMakeLists.txt
index aba9ebc..9d6ea74 100644
--- a/src/activeqt/axbase/CMakeLists.txt
+++ b/src/activeqt/axbase/CMakeLists.txt
@@ -11,6 +11,7 @@ qt_internal_add_module(AxBasePrivate
     SOURCES
         qaxtypefunctions.cpp qaxtypefunctions_p.h
         qaxutils.cpp qaxutils_p.h
+        qbstr_p.h
     LIBRARIES
         advapi32
         gdi32
diff --git a/src/activeqt/axbase/qaxutils.cpp b/src/activeqt/axbase/qaxutils.cpp
index f93910a..1645991 100644
--- a/src/activeqt/axbase/qaxutils.cpp
+++ b/src/activeqt/axbase/qaxutils.cpp
@@ -17,6 +17,8 @@
 
 #include <QtCore/qdebug.h>
 
+#include "qbstr_p.h"
+
 QT_BEGIN_NAMESPACE
 
 static inline QWindow *windowForWidget(QWidget *widget)
@@ -185,28 +187,25 @@ HRGN qaxHrgnFromQRegion(const QRegion &region, const QWidget *widget)
 QByteArray qaxTypeInfoName(ITypeInfo *typeInfo, MEMBERID memId)
 {
     QByteArray result;
-    BSTR names;
+    QBStr names;
     UINT cNames = 0;
     typeInfo->GetNames(memId, &names, 1, &cNames);
-    if (cNames && names) {
-        result = QString::fromWCharArray(names).toLatin1();
-        SysFreeString(names);
-    }
+    if (cNames && names)
+        result = names.str().toLatin1();
+
     return result;
 }
 
 QByteArrayList qaxTypeInfoNames(ITypeInfo *typeInfo, MEMBERID memId)
 {
     QByteArrayList result;
-    BSTR bstrNames[256];
+    QBStr bstrNames[256];
     UINT maxNames = 255;
     UINT maxNamesOut = 0;
     typeInfo->GetNames(memId, reinterpret_cast<BSTR *>(&bstrNames), maxNames, &maxNamesOut);
     result.reserve(maxNamesOut);
-    for (UINT p = 0; p < maxNamesOut; ++p) {
-        result.append(QString::fromWCharArray(bstrNames[p]).toLatin1());
-        SysFreeString(bstrNames[p]);
-    }
+    for (UINT p = 0; p < maxNamesOut; ++p)
+        result.append(bstrNames[p].str().toLatin1());
     return result;
 }
 
diff --git a/src/activeqt/axbase/qbstr_p.h b/src/activeqt/axbase/qbstr_p.h
new file mode 100644
index 0000000..115e3c3
--- /dev/null
+++ b/src/activeqt/axbase/qbstr_p.h
@@ -0,0 +1,119 @@
+// Copyright (C) 2024 The Qt Company Ltd.
+// SPDX-License-Identifier: LicenseRef-Qt-Commercial OR BSD-3-Clause
+
+#ifndef QBSTR_P_H
+#define QBSTR_P_H
+
+//
+//  W A R N I N G
+//  -------------
+//
+// This file is not part of the Qt API.  It exists purely as an
+// implementation detail.  This header file may change from version to
+// version without notice, or even be removed.
+//
+// We mean it.
+//
+
+#include <QtCore/QtGlobal>
+#include <QtCore/QString>
+#include <utility>
+#include <oaidl.h>
+
+class QBStr
+{
+public:
+    QBStr() = default;
+
+    ~QBStr() noexcept
+    {
+        free();
+    }
+
+    explicit QBStr(LPCOLESTR str) noexcept
+    {
+        if (str)
+            m_str = ::SysAllocString(str);
+        Q_ASSERT(m_str);
+    }
+
+    explicit QBStr(const QString &str) noexcept
+    {
+        m_str = ::SysAllocStringLen(reinterpret_cast<const OLECHAR *>(str.unicode()),
+                                    static_cast<UINT>(str.length()));
+        Q_ASSERT(m_str);
+    }
+
+    QBStr(const QBStr &str) noexcept : m_str{ str.copy() } { }
+
+    QBStr(QBStr &&str) noexcept : m_str{ std::exchange(str.m_str, nullptr) } { }
+
+    QBStr &operator=(const QBStr &rhs) noexcept
+    {
+        if (this != std::addressof(rhs))
+            reset(rhs.copy());
+
+        return *this;
+    }
+
+    QBStr &operator=(QBStr &&rhs) noexcept
+    {
+        if (this != std::addressof(rhs))
+            reset(std::exchange(rhs.m_str, nullptr));
+
+        return *this;
+    }
+
+    const BSTR &bstr() const noexcept
+    {
+        return m_str;
+    }
+
+    QString str() const
+    {
+        return QString::fromWCharArray(m_str);
+    }
+
+    [[nodiscard]] BSTR release() noexcept
+    {
+        return std::exchange(m_str, nullptr);
+    }
+
+    [[nodiscard]] BSTR *operator&() noexcept // NOLINT(google-runtime-operator)
+    {
+        Q_ASSERT(!m_str);
+        return &m_str;
+    }
+
+    explicit operator bool() const noexcept
+    {
+        return m_str != nullptr;
+    }
+
+private:
+    void free() noexcept
+    {
+        if (m_str)
+            ::SysFreeString(m_str);
+        m_str = nullptr;
+    }
+
+    void reset(BSTR str) noexcept
+    {
+        free();
+        m_str = str;
+    }
+
+    [[nodiscard]] BSTR copy() const noexcept
+    {
+        if (!m_str)
+            return nullptr;
+
+        return ::SysAllocStringByteLen(reinterpret_cast<const char *>(m_str),
+                                       ::SysStringByteLen(m_str));
+    }
+
+    BSTR m_str = nullptr;
+};
+
+#endif
diff --git a/src/activeqt/container/qaxbase.cpp b/src/activeqt/container/qaxbase.cpp
index 3463f3c..87a7930 100644
--- a/src/activeqt/container/qaxbase.cpp
+++ b/src/activeqt/container/qaxbase.cpp
@@ -32,7 +32,7 @@
 #include <private/qmetaobject_p.h>
 #include <private/qmetaobjectbuilder_p.h>
 #include <private/qtools_p.h>
-
+#include <private/qbstr_p.h>
 #include <qt_windows.h>
 #include <ocidl.h>
 #include <ctype.h>
@@ -1830,16 +1830,14 @@ QByteArray MetaObjectGenerator::usertypeToString(const TYPEDESC &tdesc, ITypeInf
         usertypeinfo->GetContainingTypeLib(&usertypelib, &index);
         if (usertypelib) {
             // get type library name
-            BSTR typelibname = nullptr;
+            QBStr typelibname;
             usertypelib->GetDocumentation(-1, &typelibname, nullptr, nullptr, nullptr);
-            QByteArray typeLibName = QString::fromWCharArray(typelibname).toLatin1();
-            SysFreeString(typelibname);
+            QByteArray typeLibName = typelibname.str().toLatin1();
 
             // get type name
-            BSTR usertypename = nullptr;
+            QBStr usertypename;
             usertypelib->GetDocumentation(INT(index), &usertypename, nullptr, nullptr, nullptr);
-            QByteArray userTypeName = QString::fromWCharArray(usertypename).toLatin1();
-            SysFreeString(usertypename);
+            QByteArray userTypeName = usertypename.str().toLatin1();
 
             if (hasEnum(userTypeName)) // known enum?
                 typeName = userTypeName;
@@ -2233,14 +2231,12 @@ void MetaObjectGenerator::readEnumInfo()
                 continue;
 
             // Get the name of the enumeration
-            BSTR enumname;
+            QBStr enumname;
             QByteArray enumName;
-            if (typelib->GetDocumentation(INT(i), &enumname, nullptr, nullptr, nullptr) == S_OK) {
-                enumName = QString::fromWCharArray(enumname).toLatin1();
-                SysFreeString(enumname);
-            } else {
+            if (typelib->GetDocumentation(INT(i), &enumname, nullptr, nullptr, nullptr) == S_OK)
+                enumName = enumname.str().toLatin1();
+            else
                 enumName = "enum" + QByteArray::number(++enum_serial);
-            }
 
             // Get the attributes of the enum type
             for (const auto &value : values) {
@@ -2949,10 +2945,9 @@ QMetaObject *MetaObjectGenerator::metaObject(const QMetaObject *parentObject, co
     if (that) {
         readClassInfo();
         if (typelib) {
-            BSTR bstr;
+            QBStr bstr;
             typelib->GetDocumentation(-1, &bstr, nullptr, nullptr, nullptr);
-            current_typelib = QString::fromWCharArray(bstr).toLatin1();
-            SysFreeString(bstr);
+            current_typelib = bstr.str().toLatin1();
         }
         if (d->tryCache && tryCache())
             return d->metaobj;
diff --git a/src/activeqt/container/qaxscript.cpp b/src/activeqt/container/qaxscript.cpp
index cc5da7d..8ca6cad 100644
--- a/src/activeqt/container/qaxscript.cpp
+++ b/src/activeqt/container/qaxscript.cpp
@@ -18,7 +18,7 @@
 #include <quuid.h>
 #include <qwidget.h>
 #include <qlist.h>
-
+#include <QtAxBase/private/qbstr_p.h>
 #include <qt_windows.h>
 #ifndef QT_NO_QAXSCRIPT
 #include <initguid.h>
@@ -246,16 +246,15 @@ HRESULT WINAPI QAxScriptSite::OnScriptError(IActiveScriptError *error)
     DWORD context;
     ULONG lineNumber;
     LONG charPos;
-    BSTR bstrLineText;
+    QBStr bstrLineText;
     QString lineText;
 
     error->GetExceptionInfo(&exception);
     error->GetSourcePosition(&context, &lineNumber, &charPos);
     HRESULT hres = error->GetSourceLineText(&bstrLineText);
-    if (hres == S_OK) {
-        lineText = QString::fromWCharArray(bstrLineText);
-        SysFreeString(bstrLineText);
-    }
+    if (hres == S_OK)
+        lineText = bstrLineText.str();
+
     SysFreeString(exception.bstrSource);
     SysFreeString(exception.bstrDescription);
     SysFreeString(exception.bstrHelpFile);
@@ -463,13 +462,14 @@ bool QAxScriptEngine::initialize(IUnknown **ptr)
         return false;
     }
 
-    BSTR bstrCode = QStringToBSTR(script_code->scriptCode());
+    QBStr bstrCode{ script_code->scriptCode() };
 #ifdef Q_OS_WIN64
-    hres = parser->ParseScriptText(bstrCode, nullptr, nullptr, nullptr, DWORDLONG(this), 0, SCRIPTTEXT_ISVISIBLE, nullptr, nullptr);
+    hres = parser->ParseScriptText(bstrCode.bstr(), nullptr, nullptr, nullptr, DWORDLONG(this), 0,
+                                   SCRIPTTEXT_ISVISIBLE, nullptr, nullptr);
 #else
-    hres = parser->ParseScriptText(bstrCode, 0, 0, 0, DWORD(this), 0, SCRIPTTEXT_ISVISIBLE, 0, 0);
+    hres = parser->ParseScriptText(bstrCode.bstr(), 0, 0, 0, DWORD(this), 0, SCRIPTTEXT_ISVISIBLE,
+                                   0, 0);
 #endif
-    SysFreeString(bstrCode);
 
     parser->Release();
     parser = nullptr;
diff --git a/tests/auto/CMakeLists.txt b/tests/auto/CMakeLists.txt
index 4cba078..6bcc7de 100644
--- a/tests/auto/CMakeLists.txt
+++ b/tests/auto/CMakeLists.txt
@@ -5,6 +5,7 @@ add_subdirectory(conversion)
 add_subdirectory(qaxobject)
 add_subdirectory(dumpcpp)
 add_subdirectory(cmake)
+add_subdirectory(qbstr)
 if(NOT GCC)
     add_subdirectory(qaxscript)
     add_subdirectory(qaxscriptmanager)
diff --git a/tests/auto/conversion/comutil_p.h b/tests/auto/conversion/comutil_p.h
index a460c3c..76cd4ac 100644
--- a/tests/auto/conversion/comutil_p.h
+++ b/tests/auto/conversion/comutil_p.h
@@ -16,6 +16,7 @@
 //
 
 #include <QtCore/QtGlobal>
+#include <QtAxBase/private/qbstr_p.h>
 #include <comdef.h>
 #include <type_traits>
 #include <oleauto.h>
@@ -31,75 +32,6 @@ ComPtr<T> makeComObject()
     return ptr;
 }
 
-class ComBstr
-{
-public:
-    ComBstr() = default;
-
-    ComBstr(decltype(nullptr)) { }
-
-    ~ComBstr() { ::SysFreeString(m_str); }
-
-    explicit ComBstr(const wchar_t *str) noexcept
-    {
-        if (!str)
-            return;
-
-        m_str = ::SysAllocString(str);
-        Q_ASSERT(m_str);
-    }
-
-    ComBstr(const ComBstr &src) noexcept
-    {
-        if (!src.m_str)
-            return;
-
-        m_str = ::SysAllocStringByteLen(reinterpret_cast<char *>(src.m_str),
-                                        ::SysStringByteLen(m_str));
-        Q_ASSERT(m_str);
-    }
-
-    ComBstr(ComBstr &&src) noexcept : m_str{ src.m_str } { src.m_str = nullptr; }
-
-    ComBstr &operator=(const ComBstr &rhs) noexcept
-    {
-        if (&rhs == this)
-            return *this;
-
-        clear();
-
-        m_str = rhs.copy();
-
-        return *this;
-    }
-
-    ComBstr &operator=(ComBstr &&rhs) noexcept
-    {
-        if (&rhs == this)
-            return *this;
-
-        clear();
-
-        m_str = rhs.m_str;
-        rhs.m_str = nullptr;
-
-        return *this;
-    }
-
-    [[nodiscard]] BSTR copy() const
-    {
-        if (!m_str)
-            return nullptr;
-
-        return ::SysAllocStringByteLen(reinterpret_cast<char *>(m_str), ::SysStringByteLen(m_str));
-    }
-
-private:
-    void clear() { ::SysFreeString(m_str); }
-
-    BSTR m_str = nullptr;
-};
-
 template<typename T>
 constexpr VARTYPE ValueType()
 {
@@ -268,7 +200,7 @@ private:
 
         constexpr auto VARIANT::*field = ValueField<T>();
         if constexpr (valueType == VT_BSTR)
-            this->*field = ComBstr{ value }.copy();
+            this->*field = QBStr{ value }.release();
         else
             this->*field = value;
 
diff --git a/tests/auto/qbstr/CMakeLists.txt b/tests/auto/qbstr/CMakeLists.txt
new file mode 100644
index 0000000..65a49a6
--- /dev/null
+++ b/tests/auto/qbstr/CMakeLists.txt
@@ -0,0 +1,15 @@
+# Copyright (C) 2024 The Qt Company Ltd.
+# SPDX-License-Identifier: BSD-3-Clause
+
+if(NOT QT_BUILD_STANDALONE_TESTS AND NOT QT_BUILDING_QT)
+    cmake_minimum_required(VERSION 3.16)
+    project(tst_qbstr LANGUAGES CXX)
+    find_package(Qt6BuildInternals REQUIRED COMPONENTS STANDALONE_TEST)
+endif()
+
+qt_internal_add_test(tst_qbstr
+    SOURCES
+        tst_qbstr.cpp
+    LIBRARIES
+        Qt::AxBasePrivate
+)
diff --git a/tests/auto/qbstr/tst_qbstr.cpp b/tests/auto/qbstr/tst_qbstr.cpp
new file mode 100644
index 0000000..8bc4f0c
--- /dev/null
+++ b/tests/auto/qbstr/tst_qbstr.cpp
@@ -0,0 +1,138 @@
+// Copyright (C) 2016 The Qt Company Ltd.
+// SPDX-License-Identifier: LicenseRef-Qt-Commercial OR GPL-3.0-only WITH Qt-GPL-exception-1.0
+
+#include <QtTest/QtTest>
+#include <QtAxBase/private/qbstr_p.h>
+
+QT_USE_NAMESPACE
+
+using namespace Qt::StringLiterals;
+
+typedef int (*SETOANOCACHE)(void);
+
+void DisableBSTRCache()
+{
+    const HINSTANCE hLib = LoadLibraryA("OLEAUT32.DLL");
+    if (hLib != nullptr) {
+        const SETOANOCACHE SetOaNoCache =
+                reinterpret_cast<SETOANOCACHE>(GetProcAddress(hLib, "SetOaNoCache"));
+        if (SetOaNoCache != nullptr)
+            SetOaNoCache();
+        FreeLibrary(hLib);
+    }
+}
+
+class tst_qbstr : public QObject
+{
+    Q_OBJECT
+private slots:
+    void initTestCase() { DisableBSTRCache(); }
+    void constructor_nullInitializes()
+    {
+        const QBStr str;
+        QVERIFY(!str.bstr());
+    }
+
+    void constructor_initializes_withLiteral()
+    {
+        const QBStr str{ L"hello world" };
+        QCOMPARE_EQ(str.bstr(), "hello world"_L1);
+    }
+
+    void constructor_initializes_withQString()
+    {
+        const QString expected{ "hello world"_L1 };
+        QBStr str{ expected };
+        QCOMPARE_EQ(QStringView{ str.bstr() }, expected);
+    }
+
+    void copyConstructor_initializes_withNullString()
+    {
+        const QBStr null;
+        const QBStr dest = null;
+        QCOMPARE_EQ(dest.bstr(), nullptr);
+    }
+
+    void copyConstructor_initializes_withValidString()
+    {
+        const QBStr expected{ L"hello world" };
+        const QBStr dest = expected;
+        QCOMPARE_EQ(QStringView{ dest.bstr() }, expected.bstr());
+    }
+
+    void moveConstructor_initializes_withNullString()
+    {
+        const QBStr str{ QBStr{} };
+        QCOMPARE_EQ(str.bstr(), nullptr);
+    }
+
+    void moveConstructor_initializes_withValidString()
+    {
+        QBStr source { L"hello world" };
+        const QBStr str{ std::move(source) };
+        QCOMPARE_EQ(str.bstr(), "hello world"_L1);
+    }
+
+    void assignment_assigns_withNullString()
+    {
+        const QBStr source{};
+        const QBStr dest = source;
+        QCOMPARE_EQ(dest.bstr(), nullptr);
+    }
+
+    void assignment_assigns_withValidString()
+    {
+        const QBStr source{ L"hello world" };
+        const QBStr dest = source;
+        QCOMPARE_EQ(dest.bstr(), "hello world"_L1);
+    }
+
+    void moveAssignment_assigns_withNullString()
+    {
+        QBStr source{};
+        const QBStr dest = std::move(source);
+        QCOMPARE_EQ(dest.bstr(), nullptr);
+    }
+
+    void moveAssignment_assigns_withValidString()
+    {
+        QBStr source{ L"hello world" };
+        const QBStr dest = std::move(source);
+        QCOMPARE_EQ(dest.bstr(), "hello world"_L1);
+    }
+
+    void release_returnsStringAndClearsValue()
+    {
+        QBStr str{ L"hello world" };
+        BSTR val = str.release();
+        QCOMPARE_EQ(str.bstr(), nullptr);
+        QCOMPARE_EQ(val, "hello world"_L1);
+        SysFreeString(val);
+    }
+
+    void str_returnsQString_withNullString()
+    {
+        const QBStr bstr{};
+        const QString str = bstr.str();
+        QVERIFY(str.isEmpty());
+    }
+
+    void str_returnsQString_withValidString()
+    {
+        const QBStr bstr{L"hello world"};
+        const QString str = bstr.str();
+        QCOMPARE_EQ(str, "hello world");
+    }
+
+    void operatorBool_returnsFalse_withNullString() {
+        QVERIFY(!QBStr{});
+    }
+
+    void operatorBool_returnsFalse_withValidString()
+    {
+        QVERIFY(QBStr{ "hello world" });
+    }
+};
+
+QTEST_MAIN(tst_qbstr)
+#include "tst_qbstr.moc"