From 51f7513bc0a77c23cb0e9ce5a7d432121900e0ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B8ger=20Hanseg=C3=A5rd?= Date: Thu, 27 Jun 2024 16:13:31 +0200 Subject: [PATCH] Replace bStrFromQString with a QBStr RAII wrapper This makes transfer of ownership explicit. The code is from ActiveQt. If this patch is accepted, it can be removed from ActiveQt. Task-number: QTBUG-126530 Change-Id: I613004ba784f87a9b935b2bbaead2205243c3033 Reviewed-by: Oliver Wolff (cherry picked from commit 93686386c078e2be03fb8bc42dee60a9e36fc23f) Reviewed-by: Qt Cherry-pick Bot --- src/corelib/CMakeLists.txt | 1 + src/corelib/platform/windows/qbstr_p.h | 144 +++++++++++ .../uiautomation/qwindowsuiamainprovider.cpp | 10 +- .../qwindowsuiatextrangeprovider.cpp | 2 +- .../windows/uiautomation/qwindowsuiautils.cpp | 7 +- .../windows/uiautomation/qwindowsuiautils.h | 3 +- .../uiautomation/qwindowsuiavalueprovider.cpp | 2 +- .../corelib/platform/windows/CMakeLists.txt | 1 + .../platform/windows/qbstr/CMakeLists.txt | 15 ++ .../platform/windows/qbstr/tst_qbstr.cpp | 227 ++++++++++++++++++ 10 files changed, 396 insertions(+), 16 deletions(-) create mode 100644 src/corelib/platform/windows/qbstr_p.h create mode 100644 tests/auto/corelib/platform/windows/qbstr/CMakeLists.txt create mode 100644 tests/auto/corelib/platform/windows/qbstr/tst_qbstr.cpp diff --git a/src/corelib/CMakeLists.txt b/src/corelib/CMakeLists.txt index 43e0b39218a..57b48ad3c07 100644 --- a/src/corelib/CMakeLists.txt +++ b/src/corelib/CMakeLists.txt @@ -562,6 +562,7 @@ qt_internal_extend_target(Core CONDITION WIN32 thread/qthread_win.cpp platform/windows/qcomobject_p.h platform/windows/qcomptr_p.h + platform/windows/qbstr_p.h LIBRARIES advapi32 authz diff --git a/src/corelib/platform/windows/qbstr_p.h b/src/corelib/platform/windows/qbstr_p.h new file mode 100644 index 00000000000..145c4a88613 --- /dev/null +++ b/src/corelib/platform/windows/qbstr_p.h @@ -0,0 +1,144 @@ +// 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 + +#if defined(Q_OS_WIN) || defined(Q_QDOC) + +#include +#include +#include +#include + +QT_BEGIN_NAMESPACE + +class QBStr +{ +public: + QBStr() = default; + + ~QBStr() noexcept + { + free(); + } + + // Does not take ownership + explicit QBStr(LPCOLESTR str) noexcept + { + if (str) + m_str = ::SysAllocString(str); + Q_ASSERT(m_str || !str); + } + + explicit QBStr(const QString &str) noexcept + { + if (!str.isNull()) + m_str = ::SysAllocString(reinterpret_cast(str.utf16())); + Q_ASSERT(m_str || str.isNull()); + } + + QBStr(const QBStr &str) noexcept : m_str{ str.copy() } { } + + QBStr(QBStr &&str) noexcept : m_str{ std::exchange(str.m_str, nullptr) } { } + + // Does not take ownership + QBStr &operator=(LPCOLESTR str) noexcept + { + free(); + if (str) + m_str = ::SysAllocString(str); + Q_ASSERT(m_str || !str); + return *this; + } + + QBStr &operator=(const QString &str) noexcept + { + free(); + if (!str.isNull()) + m_str = ::SysAllocString(reinterpret_cast(str.utf16())); + Q_ASSERT(m_str || str.isNull()); + return *this; + } + + 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; + } + +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(m_str), + ::SysStringByteLen(m_str)); + } + + BSTR m_str = nullptr; +}; + +QT_END_NAMESPACE + +#endif // Q_OS_WIN + +#endif // QCOMPTR_P_H diff --git a/src/plugins/platforms/windows/uiautomation/qwindowsuiamainprovider.cpp b/src/plugins/platforms/windows/uiautomation/qwindowsuiamainprovider.cpp index 6efefff599a..e6d72bbe05d 100644 --- a/src/plugins/platforms/windows/uiautomation/qwindowsuiamainprovider.cpp +++ b/src/plugins/platforms/windows/uiautomation/qwindowsuiamainprovider.cpp @@ -213,17 +213,15 @@ void QWindowsUiaMainProvider::raiseNotification(QAccessibleAnnouncementEvent *ev { if (QAccessibleInterface *accessible = event->accessibleInterface()) { if (auto provider = providerForAccessible(accessible)) { - BSTR message = bStrFromQString(event->message()); + QBStr message{ event->message() }; QAccessible::AnnouncementPoliteness prio = event->politeness(); NotificationProcessing processing = (prio == QAccessible::AnnouncementPoliteness::Assertive) ? NotificationProcessing_ImportantAll : NotificationProcessing_All; - BSTR activityId = bStrFromQString(QString::fromLatin1("")); - UiaRaiseNotificationEvent(provider.Get(), NotificationKind_Other, processing, message, - activityId); + QBStr activityId{ QString::fromLatin1("") }; + UiaRaiseNotificationEvent(provider.Get(), NotificationKind_Other, processing, message.bstr(), + activityId.bstr()); - ::SysFreeString(message); - ::SysFreeString(activityId); } } } diff --git a/src/plugins/platforms/windows/uiautomation/qwindowsuiatextrangeprovider.cpp b/src/plugins/platforms/windows/uiautomation/qwindowsuiatextrangeprovider.cpp index a8d58e10105..8604acff3e6 100644 --- a/src/plugins/platforms/windows/uiautomation/qwindowsuiatextrangeprovider.cpp +++ b/src/plugins/platforms/windows/uiautomation/qwindowsuiatextrangeprovider.cpp @@ -293,7 +293,7 @@ HRESULT QWindowsUiaTextRangeProvider::GetText(int maxLength, BSTR *pRetVal) if ((maxLength > -1) && (rangeText.size() > maxLength)) rangeText.truncate(maxLength); - *pRetVal = bStrFromQString(rangeText); + *pRetVal = QBStr(rangeText).release(); return S_OK; } diff --git a/src/plugins/platforms/windows/uiautomation/qwindowsuiautils.cpp b/src/plugins/platforms/windows/uiautomation/qwindowsuiautils.cpp index 78ab3e890e2..d8b788bc5a9 100644 --- a/src/plugins/platforms/windows/uiautomation/qwindowsuiautils.cpp +++ b/src/plugins/platforms/windows/uiautomation/qwindowsuiautils.cpp @@ -81,15 +81,10 @@ void setVariantDouble(double value, VARIANT *variant) variant->dblVal = value; } -BSTR bStrFromQString(const QString &value) -{ - return SysAllocString(reinterpret_cast(value.utf16())); -} - void setVariantString(const QString &value, VARIANT *variant) { variant->vt = VT_BSTR; - variant->bstrVal = bStrFromQString(value); + variant->bstrVal = QBStr(value).release(); } // Scales a rect to native coordinates, according to high dpi settings. diff --git a/src/plugins/platforms/windows/uiautomation/qwindowsuiautils.h b/src/plugins/platforms/windows/uiautomation/qwindowsuiautils.h index bf90211cec3..f4c0781f427 100644 --- a/src/plugins/platforms/windows/uiautomation/qwindowsuiautils.h +++ b/src/plugins/platforms/windows/uiautomation/qwindowsuiautils.h @@ -12,6 +12,7 @@ #include #include #include +#include #include "qwindowsuiautomation.h" QT_BEGIN_NAMESPACE @@ -38,8 +39,6 @@ void setVariantBool(bool value, VARIANT *variant); void setVariantDouble(double value, VARIANT *variant); -BSTR bStrFromQString(const QString &value); - void setVariantString(const QString &value, VARIANT *variant); } // namespace QWindowsUiAutomation diff --git a/src/plugins/platforms/windows/uiautomation/qwindowsuiavalueprovider.cpp b/src/plugins/platforms/windows/uiautomation/qwindowsuiavalueprovider.cpp index 77f40cc2cc5..a1776d6bb5f 100644 --- a/src/plugins/platforms/windows/uiautomation/qwindowsuiavalueprovider.cpp +++ b/src/plugins/platforms/windows/uiautomation/qwindowsuiavalueprovider.cpp @@ -86,7 +86,7 @@ HRESULT STDMETHODCALLTYPE QWindowsUiaValueProvider::get_Value(BSTR *pRetVal) if (!accessible) return UIA_E_ELEMENTNOTAVAILABLE; - *pRetVal = bStrFromQString(accessible->text(QAccessible::Value)); + *pRetVal = QBStr(accessible->text(QAccessible::Value)).release(); return S_OK; } diff --git a/tests/auto/corelib/platform/windows/CMakeLists.txt b/tests/auto/corelib/platform/windows/CMakeLists.txt index 24b2a69a0e5..7fe58b2ea2c 100644 --- a/tests/auto/corelib/platform/windows/CMakeLists.txt +++ b/tests/auto/corelib/platform/windows/CMakeLists.txt @@ -2,3 +2,4 @@ # SPDX-License-Identifier: BSD-3-Clause add_subdirectory(qcomobject) +add_subdirectory(qbstr) diff --git a/tests/auto/corelib/platform/windows/qbstr/CMakeLists.txt b/tests/auto/corelib/platform/windows/qbstr/CMakeLists.txt new file mode 100644 index 00000000000..75944948625 --- /dev/null +++ b/tests/auto/corelib/platform/windows/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::CorePrivate +) diff --git a/tests/auto/corelib/platform/windows/qbstr/tst_qbstr.cpp b/tests/auto/corelib/platform/windows/qbstr/tst_qbstr.cpp new file mode 100644 index 00000000000..8440b2c8df9 --- /dev/null +++ b/tests/auto/corelib/platform/windows/qbstr/tst_qbstr.cpp @@ -0,0 +1,227 @@ +// 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 + +#ifdef Q_OS_WIN + +#include + +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(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_initializesToNullptr_withNullptr() + { + const QBStr str{ nullptr }; + QCOMPARE_EQ(str.bstr(), nullptr); + } + + 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 constructor_initializesToNullptr_withNullQString() + { + const QString expected{ }; + QBStr str{ expected }; + QCOMPARE_EQ(str.bstr(), nullptr); + } + + void constructor_initializes_withEmptyQString() + { + const QString expected{ ""_L1 }; + QBStr str{ expected }; + QCOMPARE_EQ(QStringView{ str.bstr() }, expected); + QCOMPARE_NE(str.bstr(), nullptr); + } + + void copyConstructor_initializesToNullptr_withNullString() + { + const QBStr null; + const QBStr dest = null; + QCOMPARE_EQ(dest.bstr(), nullptr); + } + + void copyConstructor_initializes_withValidQBstr() + { + const QBStr expected{ L"hello world" }; + const QBStr dest = expected; + QCOMPARE_EQ(QStringView{ dest.bstr() }, expected.bstr()); + } + + void moveConstructor_initializesToNullptr_withNullQBstr() + { + const QBStr str{ QBStr{} }; + QCOMPARE_EQ(str.bstr(), nullptr); + } + + void moveConstructor_initializes_withValidQBstr() + { + QBStr source { L"hello world" }; + const QBStr str{ std::move(source) }; + QCOMPARE_EQ(str.bstr(), "hello world"_L1); + } + + void assignment_assigns_withNullptr() + { + QBStr value{}; + value = nullptr; + QCOMPARE_EQ(value.bstr(), nullptr); + } + + void assignment_assigns_WCharPointer() + { + QBStr value{}; + const wchar_t utf16[] = L"Hello world"; + const wchar_t *ptr = utf16; + value = ptr; + QCOMPARE_EQ(value.bstr(), "Hello world"_L1); + } + + void assignment_assigns_WCharLiteral() + { + QBStr value{}; + value = L"Hello world"; + QCOMPARE_EQ(value.bstr(), "Hello world"_L1); + } + + void assignment_assignsEmpty_EmptyWCharLiteral() + { + QBStr value{}; + value = L""; + QCOMPARE_EQ(value.bstr(), ""_L1); + QCOMPARE_NE(value.bstr(), nullptr); + } + + void assignment_assigns_withQString() + { + QBStr value{}; + value = QString{ "Hello world"_L1 }; + QCOMPARE_EQ(value.bstr(), "Hello world"_L1); + } + + void assignment_assignsNullptr_withNullQString() + { + QBStr value{}; + value = QString{}; + QCOMPARE_EQ(value.bstr(), nullptr); + } + + void assignment_assignsEmpty_withEmptyQString() + { + QBStr value{}; + value = QString{""_L1}; + QCOMPARE_EQ(value.bstr(), ""_L1); + QCOMPARE_NE(value.bstr(), nullptr); + } + + void assignment_assignsNullptr_withNullQBStr() + { + const QBStr source{}; + const QBStr dest = source; + QCOMPARE_EQ(dest.bstr(), nullptr); + } + + void assignment_assigns_withValidQBStr() + { + const QBStr source{ L"hello world" }; + const QBStr dest = source; + QCOMPARE_EQ(dest.bstr(), "hello world"_L1); + } + + void assignment_assigns_withSelfAssignment() + { + QBStr value{ L"hello world" }; + value = value; + QCOMPARE_EQ(value.bstr(), "hello world"_L1); + } + + void moveAssignment_assigns_withNullQBStr() + { + QBStr source{}; + const QBStr dest = std::move(source); + QCOMPARE_EQ(dest.bstr(), nullptr); + } + + void moveAssignment_assigns_withValidQBStr() + { + QBStr source{ L"hello world" }; + const QBStr dest = std::move(source); + QCOMPARE_EQ(dest.bstr(), "hello world"_L1); + } + + void moveAssignment_assigns_withSelfAssignment() + { + QBStr value{ L"hello world" }; + value = std::move(value); + QCOMPARE_EQ(value.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_returnsNullQString_withNullQBStr() + { + const QBStr bstr{}; + const QString str = bstr.str(); + QVERIFY(str.isNull()); + } + + void str_returnsQString_withValidQBStr() + { + const QBStr bstr{L"hello world"}; + const QString str = bstr.str(); + QCOMPARE_EQ(str, "hello world"); + } + +}; + +QTEST_MAIN(tst_qbstr) +#include "tst_qbstr.moc" + +#endif // Q_OS_WIN