From 57850f9d6d6cf2d745cdfcdecb4b55cd9088b898 Mon Sep 17 00:00:00 2001 From: Ulf Hermann Date: Wed, 14 Apr 2021 14:00:20 +0200 Subject: [PATCH 01/14] Switch metatypes generation on by default for Qt modules We need the metatypes for anything directly or indirectly exposed to QML. Switching this on has no runtime overhead. For interface libraries we cannot generate any metatypes, though. Change-Id: I7b7f85bb4e16c28d00383c5c88b0f1c172c8d193 Reviewed-by: Fabian Kosmale Reviewed-by: Qt CI Bot --- cmake/QtModuleHelpers.cmake | 28 +++++++++++++++++++--------- src/corelib/CMakeLists.txt | 1 + 2 files changed, 20 insertions(+), 9 deletions(-) diff --git a/cmake/QtModuleHelpers.cmake b/cmake/QtModuleHelpers.cmake index 95272d3932b..886e338b205 100644 --- a/cmake/QtModuleHelpers.cmake +++ b/cmake/QtModuleHelpers.cmake @@ -17,9 +17,10 @@ function(qt_internal_add_module target) qt_internal_module_info(module "${target}") + # TODO: Remove GENERATE_METATYPES once it's not used anymore. # Process arguments: qt_parse_all_arguments(arg "qt_add_module" - "NO_MODULE_HEADERS;STATIC;DISABLE_TOOLS_EXPORT;EXCEPTIONS;INTERNAL_MODULE;NO_SYNC_QT;NO_PRIVATE_MODULE;HEADER_MODULE;GENERATE_METATYPES;NO_CONFIG_HEADER_FILE;SKIP_DEPENDS_INCLUDE;NO_ADDITIONAL_TARGET_INFO" + "NO_MODULE_HEADERS;STATIC;DISABLE_TOOLS_EXPORT;EXCEPTIONS;INTERNAL_MODULE;NO_SYNC_QT;NO_PRIVATE_MODULE;HEADER_MODULE;GENERATE_METATYPES;NO_GENERATE_METATYPES;NO_CONFIG_HEADER_FILE;SKIP_DEPENDS_INCLUDE;NO_ADDITIONAL_TARGET_INFO" "MODULE_INCLUDE_NAME;CONFIG_MODULE_NAME;PRECOMPILED_HEADER;CONFIGURE_FILE_PATH;${__default_target_info_args}" "${__default_private_args};${__default_public_args};${__default_private_module_args};QMAKE_MODULE_CONFIG;EXTRA_CMAKE_FILES;EXTRA_CMAKE_INCLUDES;NO_PCH_SOURCES" ${ARGN}) @@ -498,15 +499,24 @@ set(QT_CMAKE_EXPORT_NAMESPACE ${QT_CMAKE_EXPORT_NAMESPACE})") endif() # Generate metatypes - if (${arg_GENERATE_METATYPES}) - set(metatypes_install_dir ${INSTALL_LIBDIR}/metatypes) - set(args) - if (NOT QT_WILL_INSTALL) - set(args COPY_OVER_INSTALL INSTALL_DIR "${QT_BUILD_DIR}/${metatypes_install_dir}") - else() - set(args INSTALL_DIR "${metatypes_install_dir}") + if(${arg_GENERATE_METATYPES}) + # No mention of NO_GENERATE_METATYPES. You should not use it. + message(WARNING "GENERATE_METATYPES is on by default for Qt modules. Please remove the manual specification.") + endif() + if (NOT ${arg_NO_GENERATE_METATYPES}) + get_target_property(target_type ${target} TYPE) + if (NOT target_type STREQUAL "INTERFACE_LIBRARY") + set(metatypes_install_dir ${INSTALL_LIBDIR}/metatypes) + set(args) + if (NOT QT_WILL_INSTALL) + set(args COPY_OVER_INSTALL INSTALL_DIR "${QT_BUILD_DIR}/${metatypes_install_dir}") + else() + set(args INSTALL_DIR "${metatypes_install_dir}") + endif() + qt6_extract_metatypes(${target} ${args}) + elseif(${arg_GENERATE_METATYPES}) + message(FATAL_ERROR "Meta types generation does not work on interface libraries") endif() - qt6_extract_metatypes(${target} ${args}) endif() qt_internal_get_min_new_policy_cmake_version(min_new_policy_version) qt_internal_get_max_new_policy_cmake_version(max_new_policy_version) diff --git a/src/corelib/CMakeLists.txt b/src/corelib/CMakeLists.txt index 3b4fdd1b465..2af8854798c 100644 --- a/src/corelib/CMakeLists.txt +++ b/src/corelib/CMakeLists.txt @@ -32,6 +32,7 @@ endif() qt_internal_add_module(Core QMAKE_MODULE_CONFIG moc resources + NO_GENERATE_METATYPES # metatypes are extracted manually below EXCEPTIONS SOURCES global/archdetect.cpp From aeeaab1a5ac0b4d91c9f9b542035b8970e4c61dd Mon Sep 17 00:00:00 2001 From: Edward Welbourne Date: Mon, 8 Feb 2021 12:13:13 +0100 Subject: [PATCH 02/14] Fix handling of surrogates in QBidiAlgorithm MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Prior code was naively assuming the character after a high surrogate would necessarily be a low surrogate, which is buggy. Fixes oss-fuzz issue 29718. Pick-to: 6.0 6.1 5.15 Change-Id: I10f023c4b5024a0d76fea0a3672001063591ec6d Reviewed-by: Konstantin Ritt Reviewed-by: Robert Löhning Reviewed-by: Lars Knoll --- src/gui/text/qtextengine.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/gui/text/qtextengine.cpp b/src/gui/text/qtextengine.cpp index 675e87b3223..b31b8806938 100644 --- a/src/gui/text/qtextengine.cpp +++ b/src/gui/text/qtextengine.cpp @@ -1,6 +1,6 @@ /**************************************************************************** ** -** Copyright (C) 2016 The Qt Company Ltd. +** Copyright (C) 2021 The Qt Company Ltd. ** Contact: https://www.qt.io/licensing/ ** ** This file is part of the QtGui module of the Qt Toolkit. @@ -258,7 +258,7 @@ struct QBidiAlgorithm { for (int i = 0; i < length; ++i) { int pos = i; char32_t uc = text[i].unicode(); - if (QChar::isHighSurrogate(uc) && i < length - 1) { + if (QChar::isHighSurrogate(uc) && i < length - 1 && text[i + 1].isLowSurrogate()) { ++i; analysis[i].bidiDirection = QChar::DirNSM; uc = QChar::surrogateToUcs4(ushort(uc), text[i].unicode()); From 8546e017c01311b2625d03ef7ca3dfdd491eb88b Mon Sep 17 00:00:00 2001 From: Giuseppe D'Angelo Date: Thu, 15 Apr 2021 14:38:36 +0200 Subject: [PATCH 03/14] util/unicode: enable asserts unconditionally If one "accidentally" uses a release build of the unicode tool, the asserts within it won't fire. Enable them in all cases. Change-Id: I9d63641dc6d6d2e5805b61b36f8c28e624b25e12 Reviewed-by: Edward Welbourne --- util/unicode/unicode.pro | 1 + 1 file changed, 1 insertion(+) diff --git a/util/unicode/unicode.pro b/util/unicode/unicode.pro index 0250c2a43a2..a8c941cf8cd 100644 --- a/util/unicode/unicode.pro +++ b/util/unicode/unicode.pro @@ -1,3 +1,4 @@ SOURCES += main.cpp QT = core CONFIG += console +DEFINES += QT_FORCE_ASSERTS From b322bfcc14845a4b6a6eef85ef359b1e4591a5ca Mon Sep 17 00:00:00 2001 From: Giuseppe D'Angelo Date: Thu, 15 Apr 2021 14:31:46 +0200 Subject: [PATCH 04/14] QString: add missing char8_t* constructor / fromUtf8 overloads Currently we support: QString s = QString::fromUtf(u8"foo", 3); But we don't support QString s = QString::fromUtf8(u8"foo"); QString s(u8"foo"); There's no reason not to have these two functions. Guess what, we've actually got code _in Qt_ that tries to build a QString out of a char8_t; that code stops compiling under C++20 (which is supported, but not CI-tested at the moment, it seems). Re-add the missing constructor and fromUtf8 overloads. [ChangeLog][QtCore][QString] Added a constructor and a fromUtf8() overload taking a `const char8_t *` argument. Task-number: QTQAINFRA-4117 Task-number: QTQAINFRA-4242 Change-Id: I1f0ae658b3490b9e092941cabcc7fb8fc4c51aa3 Pick-to: 6.1.0 6.1 Reviewed-by: Lars Knoll Reviewed-by: Volker Hilsheimer Reviewed-by: Edward Welbourne --- src/corelib/text/qstring.cpp | 18 ++++++++++++++++++ src/corelib/text/qstring.h | 9 +++++++++ 2 files changed, 27 insertions(+) diff --git a/src/corelib/text/qstring.cpp b/src/corelib/text/qstring.cpp index a690aac9360..5a340530a93 100644 --- a/src/corelib/text/qstring.cpp +++ b/src/corelib/text/qstring.cpp @@ -2190,6 +2190,16 @@ inline char qToLower(char ch) \sa fromLatin1(), fromLocal8Bit(), fromUtf8() */ +/*! \fn QString::QString(const char8_t *str) + + Constructs a string initialized with the UTF-8 string \a str. The + given const char8_t pointer is converted to Unicode using the + fromUtf8() function. + + \since 6.1 + \sa fromLatin1(), fromLocal8Bit(), fromUtf8() +*/ + /*! \fn QString QString::fromStdString(const std::string &str) Returns a copy of the \a str string. The given string is converted @@ -5324,6 +5334,14 @@ QString QString::fromLocal8Bit(QByteArrayView ba) \sa toUtf8(), fromLatin1(), fromLocal8Bit() */ +/*! + \fn QString QString::fromUtf8(const char8_t *str) + \overload + \since 6.1 + + This overload is only available when compiling in C++20 mode. +*/ + /*! \fn QString QString::fromUtf8(const char8_t *str, qsizetype size) \overload diff --git a/src/corelib/text/qstring.h b/src/corelib/text/qstring.h index c6d97c9d513..5e215625c6b 100644 --- a/src/corelib/text/qstring.h +++ b/src/corelib/text/qstring.h @@ -386,6 +386,12 @@ public: QString(QChar c); QString(qsizetype size, QChar c); inline QString(QLatin1String latin1); +#if defined(__cpp_char8_t) || defined(Q_CLANG_QDOC) + Q_WEAK_OVERLOAD + inline QString(const char8_t *str) + : QString(fromUtf8(str)) + {} +#endif inline QString(const QString &) noexcept; inline ~QString(); QString &operator=(QChar c); @@ -746,6 +752,9 @@ public: return fromUtf8(QByteArrayView(utf8, !utf8 || size < 0 ? qstrlen(utf8) : size)); } #if defined(__cpp_char8_t) || defined(Q_CLANG_QDOC) + Q_WEAK_OVERLOAD + static inline QString fromUtf8(const char8_t *str) + { return fromUtf8(reinterpret_cast(str)); } Q_WEAK_OVERLOAD static inline QString fromUtf8(const char8_t *str, qsizetype size) { return fromUtf8(reinterpret_cast(str), size); } From 5656a60dd067a69f9e864a33068ec300124d4e05 Mon Sep 17 00:00:00 2001 From: Giuseppe D'Angelo Date: Thu, 15 Apr 2021 16:10:12 +0200 Subject: [PATCH 05/14] QList::(const_)iterator: protect element_type on GCC < 11 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GCC 10 in C++20 mode will still try to use std::indirectly_readable_traits on QList iterators, and since it does not have the fixes for LWG 3346 / 3541, it will fail to build: /usr/include/c++/10/bits/iterator_concepts.h: In substitution of ‘template using __iter_value_t = typename std::__detail::__iter_traits_impl<_Tp, std::indirectly_readable_traits<_Iter> >::type::value_type [with _Tp = QList >::const_iterator]’: /usr/include/c++/10/bits/iterator_concepts.h:248:11: required by substitution of ‘template using iter_value_t = std::__detail::__iter_value_t::type>::type> [with _Tp = QList >::const_iterator]’ /usr/include/c++/10/bits/iterator_concepts.h:468:11: required from ‘class std::reverse_iterator >::const_iterator>’ ../src/corelib/itemmodels/qsortfilterproxymodel.cpp:669:43: required from here /usr/include/c++/10/bits/iterator_concepts.h:243:13: error: ambiguous template instantiation for ‘struct std::indirectly_readable_traits >::const_iterator>’ 243 | using __iter_value_t = typename | ^~~~~~~~~~~~~~ /usr/include/c++/10/bits/iterator_concepts.h:231:12: note: candidates are: ‘template requires requires{typename _Tp::value_type;} struct std::indirectly_readable_traits<_Iter> [with _Tp = QList >::const_iterator]’ 231 | struct indirectly_readable_traits<_Tp> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /usr/include/c++/10/bits/iterator_concepts.h:236:12: note: ‘template requires requires{typename _Tp::element_type;} struct std::indirectly_readable_traits<_Iter> [with _Tp = QList >::const_iterator]’ 236 | struct indirectly_readable_traits<_Tp> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ So hide element_type as well. The CI didn't catch this because the CI doesn't build in C++20 mode. Amends 595b4e1a9b4. Change-Id: I5c8e47d693ca584571cd89f856d08ba249dd05ab Reviewed-by: Fabian Kosmale --- src/corelib/tools/qlist.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/corelib/tools/qlist.h b/src/corelib/tools/qlist.h index 61a02f795ba..6aafb86c01e 100644 --- a/src/corelib/tools/qlist.h +++ b/src/corelib/tools/qlist.h @@ -137,10 +137,10 @@ public: // libstdc++ shipped with gcc < 11 does not have a fix for defect LWG 3346 #if __cplusplus >= 202002L && (!defined(_GLIBCXX_RELEASE) || _GLIBCXX_RELEASE >= 11) using iterator_category = std::contiguous_iterator_tag; + using element_type = value_type; #else using iterator_category = std::random_access_iterator_tag; #endif - using element_type = value_type; using pointer = T *; using reference = T &; @@ -179,10 +179,10 @@ public: // libstdc++ shipped with gcc < 11 does not have a fix for defect LWG 3346 #if __cplusplus >= 202002L && (!defined(_GLIBCXX_RELEASE) || _GLIBCXX_RELEASE >= 11) using iterator_category = std::contiguous_iterator_tag; + using element_type = const value_type; #else using iterator_category = std::random_access_iterator_tag; #endif - using element_type = const value_type; using pointer = const T *; using reference = const T &; From f4417bf7e8f843438345f496cf5d6a9b6fa33709 Mon Sep 17 00:00:00 2001 From: Joerg Bornemann Date: Thu, 15 Apr 2021 16:41:02 +0200 Subject: [PATCH 06/14] Check whether CMake was built with zstd support CMake 3.18 introduced the file(ARCHIVE_CREATE) API that we use with COMPRESSION Zstd for compressing corelib's mimedatabase. It's possible to build CMake without proper zstd support, and we have encountered such builds in the wild where the file(ARCHIVE_CREATE) call crashes. Add a configure test to determine whether CMake properly supports the Zstd compression method. Fixes: QTBUG-89108 Change-Id: I37e389c878845162b6f18457984d4f73a265b604 Reviewed-by: Alexandru Croitor --- config.tests/cmake_zstd/check_zstd.cmake | 5 +++++ configure.cmake | 18 ++++++++++++++++++ src/corelib/CMakeLists.txt | 5 +++++ 3 files changed, 28 insertions(+) create mode 100644 config.tests/cmake_zstd/check_zstd.cmake diff --git a/config.tests/cmake_zstd/check_zstd.cmake b/config.tests/cmake_zstd/check_zstd.cmake new file mode 100644 index 00000000000..267494f90b1 --- /dev/null +++ b/config.tests/cmake_zstd/check_zstd.cmake @@ -0,0 +1,5 @@ +file(ARCHIVE_CREATE + OUTPUT cmake_zstd.zstd + PATHS "${CMAKE_CURRENT_LIST_FILE}" + FORMAT raw + COMPRESSION Zstd) diff --git a/configure.cmake b/configure.cmake index 840d5f586cb..cfd355dcfe2 100644 --- a/configure.cmake +++ b/configure.cmake @@ -830,6 +830,24 @@ qt_feature("zstd" PRIVATE LABEL "Zstandard support" CONDITION ZSTD_FOUND ) +# special case begin +# Check whether CMake was built with zstd support. +# See https://gitlab.kitware.com/cmake/cmake/-/issues/21552 +if(NOT DEFINED CACHE{QT_CMAKE_ZSTD_SUPPORT}) + set(QT_CMAKE_ZSTD_SUPPORT FALSE CACHE INTERNAL "") + if(CMAKE_VERSION VERSION_GREATER_EQUAL "3.18") + execute_process(COMMAND "${CMAKE_COMMAND}" + -P "${CMAKE_CURRENT_SOURCE_DIR}/config.tests/cmake_zstd/check_zstd.cmake" + WORKING_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}/config.tests" + OUTPUT_QUIET ERROR_QUIET + RESULT_VARIABLE qt_check_zstd_exit_code) + if(qt_check_zstd_exit_code EQUAL 0) + set(QT_CMAKE_ZSTD_SUPPORT TRUE CACHE INTERNAL "") + endif() + unset(qt_check_zstd_exit_code) + endif() +endif() +# special case end qt_feature("thread" PUBLIC SECTION "Kernel" LABEL "Thread support" diff --git a/src/corelib/CMakeLists.txt b/src/corelib/CMakeLists.txt index 2af8854798c..3c6b20636a5 100644 --- a/src/corelib/CMakeLists.txt +++ b/src/corelib/CMakeLists.txt @@ -1187,6 +1187,11 @@ if(QT_FEATURE_mimetype AND QT_FEATURE_mimetype_database) ) else() if(QT_FEATURE_zstd) + if(NOT QT_CMAKE_ZSTD_SUPPORT) + message(FATAL_ERROR + "CMake was not built with zstd support. " + "Rebuild CMake or set QT_AVOID_CMAKE_ARCHIVING_API=ON.") + endif() set(qmime_db_compression Zstd) string(APPEND qmime_db_content "#define MIME_DATABASE_IS_ZSTD\n") else() From e27390f8e1fb961e783ce1004b3a8caf5eeaeca6 Mon Sep 17 00:00:00 2001 From: Fabian Kosmale Date: Fri, 16 Apr 2021 14:29:08 +0200 Subject: [PATCH 07/14] QMultiHash: Fix doc In Qt 6, QMultiHash does not iherit QHash Pick-to: 6.1 6.0 Change-Id: Iaad8768d681a9aad2bb1f80fd87904f0dd9683d4 Reviewed-by: Paul Wicking --- src/corelib/tools/qhash.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/corelib/tools/qhash.cpp b/src/corelib/tools/qhash.cpp index 092985887f6..51e2a18961b 100644 --- a/src/corelib/tools/qhash.cpp +++ b/src/corelib/tools/qhash.cpp @@ -2578,11 +2578,10 @@ size_t qHash(long double key, size_t seed) noexcept hashes. A multi-valued hash is a hash that allows multiple values with the same key. - Because QMultiHash inherits QHash, all of QHash's functionality also - applies to QMultiHash. For example, you can use isEmpty() to test + QMultiHash mostly mirrors QHash's API. For example, you can use isEmpty() to test whether the hash is empty, and you can traverse a QMultiHash using QHash's iterator classes (for example, QHashIterator). But opposed to - QHash, it provides an insert() function will allow the insertion of + QHash, it provides an insert() function that allows the insertion of multiple items with the same key. The replace() function corresponds to QHash::insert(). It also provides convenient operator+() and operator+=(). From 6ed83f82fe48d9ea9ab6c2a984f4bb6d51acf6a7 Mon Sep 17 00:00:00 2001 From: Volker Hilsheimer Date: Fri, 16 Apr 2021 11:30:51 +0200 Subject: [PATCH 08/14] Port example away from deprecated QVariant API Change-Id: I284610f216409b593d307b8076c8f638722929e6 Reviewed-by: Fabian Kosmale --- examples/widgets/tools/settingseditor/settingstree.cpp | 2 +- src/corelib/kernel/qvariant.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/examples/widgets/tools/settingseditor/settingstree.cpp b/examples/widgets/tools/settingseditor/settingstree.cpp index 04af8ce3b96..bcbf3744a50 100644 --- a/examples/widgets/tools/settingseditor/settingstree.cpp +++ b/examples/widgets/tools/settingseditor/settingstree.cpp @@ -214,7 +214,7 @@ void SettingsTree::updateChildItems(QTreeWidgetItem *parent) if (value.userType() == QMetaType::UnknownType) { child->setText(1, "Invalid"); } else { - if (value.type() == QVariant::String) { + if (value.typeId() == QMetaType::QString) { const QString stringValue = value.toString(); if (m_typeChecker->boolExp.match(stringValue).hasMatch()) { value.setValue(stringValue.compare("true", Qt::CaseInsensitive) == 0); diff --git a/src/corelib/kernel/qvariant.h b/src/corelib/kernel/qvariant.h index d95d3cd0fb8..9ebdbc4a18f 100644 --- a/src/corelib/kernel/qvariant.h +++ b/src/corelib/kernel/qvariant.h @@ -333,7 +333,7 @@ class Q_CORE_EXPORT QVariant explicit QVariant(Type type) : QVariant(QMetaType(int(type))) {} - QT_DEPRECATED_VERSION_X_6_0("Use metaType().") + QT_DEPRECATED_VERSION_X_6_0("Use typeId() or metaType().") Type type() const { int type = d.typeId(); From f641a0dbcfd52e78e068357d9c66236798e043e8 Mon Sep 17 00:00:00 2001 From: Joerg Bornemann Date: Thu, 15 Apr 2021 13:35:14 +0200 Subject: [PATCH 09/14] Fix build of jpeg plugin against recent jpeg-turbo on MinGW Remove the MinGW-related hack that was introduced in ec31953007126a6e0f9f3ca16b64bdfdcdf3d7b6. It doesn't seem to be needed anymore with recent MinGW versions and prevents using a system jpeglib that disagrees in its jboolean declaration with our bundled jpeglib. Fixes: QTBUG-88093 Change-Id: Ic6eb03b4b395fe3e8dcedf52489e8642289fc98e Reviewed-by: Kai Koehne Reviewed-by: Qt CI Bot --- src/plugins/imageformats/jpeg/qjpeghandler.cpp | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/plugins/imageformats/jpeg/qjpeghandler.cpp b/src/plugins/imageformats/jpeg/qjpeghandler.cpp index beef18f2601..a17afc0f696 100644 --- a/src/plugins/imageformats/jpeg/qjpeghandler.cpp +++ b/src/plugins/imageformats/jpeg/qjpeghandler.cpp @@ -61,13 +61,6 @@ // including jpeglib.h seems to be a little messy extern "C" { -// jpeglib.h->jmorecfg.h tries to typedef int boolean; but this conflicts with -// some Windows headers that may or may not have been included -#ifdef HAVE_BOOLEAN -# undef HAVE_BOOLEAN -#endif -#define boolean jboolean - #define XMD_H // shut JPEGlib up #include #ifdef const From d72067c93833e9787a45aeddee800faab0d2d40a Mon Sep 17 00:00:00 2001 From: Assam Boudjelthia Date: Fri, 16 Apr 2021 10:51:41 +0300 Subject: [PATCH 10/14] Add documentation links for some JNI entities Add doc page link for: * AttachCurrentThread call. * Interface Function Table which describes JNIEnv. Pick-to: 6.1 6.1.0 Change-Id: I12b41429c40838e5133e58132930aede287e2e71 Reviewed-by: Alex Blasche --- src/corelib/doc/src/external-resources.qdoc | 12 +++++++++++- src/corelib/kernel/qjnienvironment.cpp | 6 ++++-- src/corelib/kernel/qjniobject.cpp | 2 +- 3 files changed, 16 insertions(+), 4 deletions(-) diff --git a/src/corelib/doc/src/external-resources.qdoc b/src/corelib/doc/src/external-resources.qdoc index 583e668be46..abd359ac338 100644 --- a/src/corelib/doc/src/external-resources.qdoc +++ b/src/corelib/doc/src/external-resources.qdoc @@ -78,7 +78,7 @@ /*! \externalpage https://docs.oracle.com/javase/7/docs/technotes/guides/jni/spec/functions.html - \title Oracle: JNI Functions + \title Java: JNI Functions */ /*! @@ -90,3 +90,13 @@ \externalpage https://developer.android.com/training/articles/perf-jni#local-and-global-references \title JNI tips: Local and global references */ + +/*! + \externalpage https://docs.oracle.com/javase/7/docs/technotes/guides/jni/spec/invocation.html#attach_current_thread + \title Java: AttachCurrentThread +*/ + +/*! + \externalpage https://docs.oracle.com/en/java/javase/13/docs/specs/jni/functions.html#interface-function-table + \title Java: Interface Function Table +*/ diff --git a/src/corelib/kernel/qjnienvironment.cpp b/src/corelib/kernel/qjnienvironment.cpp index 522c97bdc15..f7a5fcb6862 100644 --- a/src/corelib/kernel/qjnienvironment.cpp +++ b/src/corelib/kernel/qjnienvironment.cpp @@ -60,6 +60,8 @@ QT_BEGIN_NAMESPACE Since \c JNIEnv doesn't do much error checking, such as exception checking and clearing, QJniEnvironment allows you to do that easily. + For more information about JNIEnv, see \l {Java: Interface Function Table}. + \note This API has been designed and tested for use with Android. It has not been tested for other platforms. */ @@ -266,7 +268,7 @@ bool QJniEnvironment::registerNativeMethods(const char *className, JNINativeMeth In contrast to \l QJniObject, which handles exceptions internally, if you make JNI calls directly via \c JNIEnv, you need to clear any potential exceptions after the call using this function. For more information about - \c JNIEnv calls that can throw an exception, see \l {Oracle: JNI Functions}{JNI Functions}. + \c JNIEnv calls that can throw an exception, see \l {Java: JNI Functions}{JNI Functions}. \return \c true when a pending exception was cleared. */ @@ -293,7 +295,7 @@ bool QJniEnvironment::checkAndClearExceptions(QJniEnvironment::OutputMode output In contrast to \l QJniObject, which handles exceptions internally, if you make JNI calls directly via \c JNIEnv, you need to clear any potential exceptions after the call using this function. For more information about - \c JNIEnv calls that can throw an exception, see \l {Oracle: JNI Functions}{JNI Functions}. + \c JNIEnv calls that can throw an exception, see \l {Java: JNI Functions}{JNI Functions}. \return \c true when a pending exception was cleared. */ diff --git a/src/corelib/kernel/qjniobject.cpp b/src/corelib/kernel/qjniobject.cpp index 9bd71c061ed..418a404bb4f 100644 --- a/src/corelib/kernel/qjniobject.cpp +++ b/src/corelib/kernel/qjniobject.cpp @@ -160,7 +160,7 @@ QT_BEGIN_NAMESPACE \l {JNI Design Overview: Global and Local References}. Local references created outside a native method scope must be deleted manually, since the garbage collector will not free them automatically because we are using - \c AttachCurrentThread. For more information, see + \l {Java: AttachCurrentThread}{AttachCurrentThread}. For more information, see \l {JNI tips: Local and global references}. If you want to keep a Java object alive you need to either create a new global From cf42a0fe5e525efa9a399694cc6882c6e811c286 Mon Sep 17 00:00:00 2001 From: Lars Knoll Date: Wed, 3 Feb 2021 10:16:18 +0100 Subject: [PATCH 11/14] Remove lazy binding evaluation Too much of the existing code in Qt requires eager evaluation without large scale modifications. Combined with the fact that supporting both eager and lazy evaluation has a high maintenance burden, keeping lazy evaluation, at least in its current state, is not worth it. This does not diminish other benefits of the new property system, which include - a C++ API to setup and modify bindings and - faster execution compared to QML's existing bindings and the ability to use them without having a QML engine. We do no longer benefit from doing less work thanks to laziness. A later commit will introduce grouping support to recapture some of this benefit. [ChangeLog][Import Behavior Change][QProperty] QProperty uses always eager evaluation now when a dependency in a binding changes. Change-Id: I34694fd5c7bcb1d31a0052d2e3da8b68d016671b Reviewed-by: Fabian Kosmale Reviewed-by: Lars Knoll Reviewed-by: Andreas Buhr Reviewed-by: Andrei Golubev --- .../src/objectmodel/bindableproperties.qdoc | 16 +-- src/corelib/kernel/qproperty.cpp | 129 ++++++------------ src/corelib/kernel/qproperty.h | 42 ++---- src/corelib/kernel/qproperty_p.h | 37 +---- src/corelib/kernel/qpropertyprivate.h | 3 +- src/corelib/kernel/qtimer.cpp | 7 +- .../kernel/qproperty/tst_qproperty.cpp | 98 ++----------- 7 files changed, 82 insertions(+), 250 deletions(-) diff --git a/src/corelib/doc/src/objectmodel/bindableproperties.qdoc b/src/corelib/doc/src/objectmodel/bindableproperties.qdoc index 8b50c65d7c9..1bb20935d0f 100644 --- a/src/corelib/doc/src/objectmodel/bindableproperties.qdoc +++ b/src/corelib/doc/src/objectmodel/bindableproperties.qdoc @@ -50,8 +50,7 @@ The binding expression computes the value by reading other QProperty values. Behind the scenes this dependency is tracked. Whenever a change in any property's dependency is detected, the binding expression is re-evaluated and the new - result is applied to the property. This happens lazily, by marking the binding - as dirty and evaluating it only when the property's value is requested. For example: + result is applied to the property. For example: \code QProperty firstname("John"); @@ -63,20 +62,19 @@ qDebug() << fullname.value(); // Prints "John Smith age: 41" - firstname = "Emma"; // Marks binding expression as dirty + firstname = "Emma"; // Triggers binding reevaluation - qDebug() << fullname.value(); // Re-evaluates the binding expression and prints "Emma Smith age: 41" + qDebug() << fullname.value(); // Prints the new value "Emma Smith age: 41" // Birthday is coming up - age.setValue(age.value() + 1); + age.setValue(age.value() + 1); // Triggers re-evaluation - qDebug() << fullname.value(); // Re-evaluates the binding expression and prints "Emma Smith age: 42" + qDebug() << fullname.value(); // Prints "Emma Smith age: 42" \endcode When a new value is assigned to the \c firstname property, the binding - expression for \c fullname is marked as dirty. So when the last \c qDebug() statement - tries to read the name value of the \c fullname property, the expression is - evaluated again, \c firstname() will be called again and return the new value. + expression for \c fullname is reevaluated. So when the last \c qDebug() statement + tries to read the name value of the \c fullname property, the new value is returned. Since bindings are C++ functions, they may do anything that's possible in C++. This includes calling other functions. If those functions access values diff --git a/src/corelib/kernel/qproperty.cpp b/src/corelib/kernel/qproperty.cpp index d03991eac17..ad254922832 100644 --- a/src/corelib/kernel/qproperty.cpp +++ b/src/corelib/kernel/qproperty.cpp @@ -98,44 +98,17 @@ void QPropertyBindingPrivate::unlinkAndDeref() destroyAndFreeMemory(this); } -void QPropertyBindingPrivate::markDirtyAndNotifyObservers() +void QPropertyBindingPrivate::evaluate() { - if (eagerlyUpdating) { - error = QPropertyBindingError(QPropertyBindingError::BindingLoop); - if (isQQmlPropertyBinding) - errorCallBack(this); - return; - } - if (dirty) - return; - dirty = true; - - eagerlyUpdating = true; - QScopeGuard guard([&](){eagerlyUpdating = false;}); - bool knownToHaveChanged = false; - if (requiresEagerEvaluation()) { - // these are compat properties that we will need to evaluate eagerly - if (!evaluateIfDirtyAndReturnTrueIfValueChanged(propertyDataPtr)) - return; - knownToHaveChanged = true; - } - if (firstObserver) - firstObserver.notify(this, propertyDataPtr, knownToHaveChanged); - if (hasStaticObserver) - staticObserverCallback(propertyDataPtr); -} - -bool QPropertyBindingPrivate::evaluateIfDirtyAndReturnTrueIfValueChanged_helper(const QUntypedPropertyData *data, QBindingStatus *status) -{ - Q_ASSERT(dirty); - if (updating) { error = QPropertyBindingError(QPropertyBindingError::BindingLoop); if (isQQmlPropertyBinding) errorCallBack(this); - return false; + return; } + QScopedValueRollback updateGuard(updating, true); + /* * Evaluating the binding might lead to the binding being broken. This can * cause ref to reach zero at the end of the function. However, the @@ -145,23 +118,26 @@ bool QPropertyBindingPrivate::evaluateIfDirtyAndReturnTrueIfValueChanged_helper( * that the object is still alive when updateGuard's dtor runs. */ QPropertyBindingPrivatePtr keepAlive {this}; - QScopedValueRollback updateGuard(updating, true); - BindingEvaluationState evaluationFrame(this, status); - - bool changed = false; - - Q_ASSERT(propertyDataPtr == data); - QUntypedPropertyData *mutable_data = const_cast(data); + BindingEvaluationState evaluationFrame(this); + bool changed; + auto bindingFunctor = reinterpret_cast(this) + + QPropertyBindingPrivate::getSizeEnsuringAlignment(); if (hasBindingWrapper) { - changed = staticBindingWrapper(metaType, mutable_data, {vtable, reinterpret_cast(this)+QPropertyBindingPrivate::getSizeEnsuringAlignment()}); + changed = staticBindingWrapper(metaType, propertyDataPtr, + {vtable, bindingFunctor}); } else { - changed = vtable->call(metaType, mutable_data, reinterpret_cast(this)+ QPropertyBindingPrivate::getSizeEnsuringAlignment()); + changed = vtable->call(metaType, propertyDataPtr, bindingFunctor); } + if (!changed) + return; - dirty = false; - return changed; + // notify dependent bindings and emit changed signal + if (firstObserver) + firstObserver.notify(propertyDataPtr); + if (hasStaticObserver) + staticObserverCallback(propertyDataPtr); } QUntypedPropertyBinding::QUntypedPropertyBinding() = default; @@ -250,7 +226,7 @@ QUntypedPropertyBinding QPropertyBindingData::setBinding(const QUntypedPropertyB if (auto *existingBinding = d.bindingPtr()) { if (existingBinding == newBinding.data()) return QUntypedPropertyBinding(static_cast(oldBinding.data())); - if (existingBinding->isEagerlyUpdating()) { + if (existingBinding->isUpdating()) { existingBinding->setError({QPropertyBindingError::BindingLoop, QStringLiteral("Binding set during binding evaluation!")}); return QUntypedPropertyBinding(static_cast(oldBinding.data())); } @@ -267,18 +243,12 @@ QUntypedPropertyBinding QPropertyBindingData::setBinding(const QUntypedPropertyB d_ptr = reinterpret_cast(newBinding.data()); d_ptr |= BindingBit; auto newBindingRaw = static_cast(newBinding.data()); - newBindingRaw->setDirty(true); newBindingRaw->setProperty(propertyDataPtr); if (observer) newBindingRaw->prependObserver(observer); newBindingRaw->setStaticObserver(staticObserverCallback, guardCallback); - if (newBindingRaw->requiresEagerEvaluation()) { - newBindingRaw->setEagerlyUpdating(true); - auto changed = newBindingRaw->evaluateIfDirtyAndReturnTrueIfValueChanged(propertyDataPtr); - if (changed) - observer.notify(newBindingRaw, propertyDataPtr, /*knownToHaveChanged=*/true); - newBindingRaw->setEagerlyUpdating(false); - } + + newBindingRaw->evaluate(); } else if (observer) { d.setObservers(observer.ptr); } else { @@ -333,13 +303,9 @@ QPropertyBindingPrivate *QPropertyBindingPrivate::currentlyEvaluatingBinding() return currentState ? currentState->binding : nullptr; } -void QPropertyBindingData::evaluateIfDirty(const QUntypedPropertyData *property) const +// ### Unused, kept for BC with 6.0 +void QPropertyBindingData::evaluateIfDirty(const QUntypedPropertyData *) const { - QPropertyBindingDataPointer d{this}; - QPropertyBindingPrivate *binding = d.bindingPtr(); - if (!binding) - return; - binding->evaluateIfDirtyAndReturnTrueIfValueChanged(property); } void QPropertyBindingData::removeBinding_helper() @@ -370,7 +336,7 @@ void QPropertyBindingData::registerWithCurrentlyEvaluatingBinding_helper(Binding QPropertyBindingDataPointer d{this}; QPropertyObserverPointer dependencyObserver = currentState->binding->allocateDependencyObserver(); - dependencyObserver.setBindingToMarkDirty(currentState->binding); + dependencyObserver.setBindingToNotify(currentState->binding); dependencyObserver.observeProperty(d); } @@ -378,14 +344,7 @@ void QPropertyBindingData::notifyObservers(QUntypedPropertyData *propertyDataPtr { QPropertyBindingDataPointer d{this}; if (QPropertyObserverPointer observer = d.firstObserver()) - observer.notify(d.bindingPtr(), propertyDataPtr); -} - -void QPropertyBindingData::markDirty() -{ - QPropertyBindingDataPointer d{this}; - if (auto *binding = d.bindingPtr()) - binding->setDirty(true); + observer.notify(propertyDataPtr); } int QPropertyBindingDataPointer::observerCount() const @@ -425,7 +384,7 @@ QPropertyObserver::~QPropertyObserver() QPropertyObserver::QPropertyObserver(QPropertyObserver &&other) noexcept { - bindingToMarkDirty = std::exchange(other.bindingToMarkDirty, {}); + binding = std::exchange(other.binding, {}); next = std::exchange(other.next, {}); prev = std::exchange(other.prev, {}); if (next) @@ -441,9 +400,9 @@ QPropertyObserver &QPropertyObserver::operator=(QPropertyObserver &&other) noexc QPropertyObserverPointer d{this}; d.unlink(); - bindingToMarkDirty = nullptr; + binding = nullptr; - bindingToMarkDirty = std::exchange(other.bindingToMarkDirty, {}); + binding = std::exchange(other.binding, {}); next = std::exchange(other.next, {}); prev = std::exchange(other.prev, {}); if (next) @@ -480,10 +439,10 @@ void QPropertyObserverPointer::setAliasedProperty(QUntypedPropertyData *property ptr->next.setTag(QPropertyObserver::ObserverNotifiesAlias); } -void QPropertyObserverPointer::setBindingToMarkDirty(QPropertyBindingPrivate *binding) +void QPropertyObserverPointer::setBindingToNotify(QPropertyBindingPrivate *binding) { Q_ASSERT(ptr->next.tag() != QPropertyObserver::ObserverIsPlaceholder); - ptr->bindingToMarkDirty = binding; + ptr->binding = binding; ptr->next.setTag(QPropertyObserver::ObserverNotifiesBinding); } @@ -516,14 +475,8 @@ struct [[nodiscard]] QPropertyObserverNodeProtector { /*! \internal \a propertyDataPtr is a pointer to the observed property's property data - In case that property has a binding, \a triggeringBinding points to the binding's QPropertyBindingPrivate - \a alreadyKnownToHaveChanged is an optional parameter, which is needed in the case - of eager evaluation: - There, we have already evaluated the binding, and thus the change detection for the - ObserverNotifiesChangeHandler case would not work. Thus we instead pass the knowledge of - whether the value has changed we obtained when evaluating the binding eagerly along */ -void QPropertyObserverPointer::notify(QPropertyBindingPrivate *triggeringBinding, QUntypedPropertyData *propertyDataPtr, bool knownToHaveChanged) +void QPropertyObserverPointer::notify(QUntypedPropertyData *propertyDataPtr) { auto observer = const_cast(ptr); /* @@ -557,22 +510,17 @@ void QPropertyObserverPointer::notify(QPropertyBindingPrivate *triggeringBinding observer = next->next.data(); continue; } - // both evaluateIfDirtyAndReturnTrueIfValueChanged and handlerToCall might modify the list + // handlerToCall might modify the list QPropertyObserverNodeProtector protector(observer); - if (!knownToHaveChanged && triggeringBinding) { - if (!triggeringBinding->evaluateIfDirtyAndReturnTrueIfValueChanged(propertyDataPtr)) - return; - knownToHaveChanged = true; - } handlerToCall(observer, propertyDataPtr); next = protector.next(); break; } case QPropertyObserver::ObserverNotifiesBinding: { - auto bindingToMarkDirty = observer->bindingToMarkDirty; + auto bindingToMarkDirty = observer->binding; QPropertyObserverNodeProtector protector(observer); - bindingToMarkDirty->markDirtyAndNotifyObservers(); + bindingToMarkDirty->evaluate(); next = protector.next(); break; } @@ -1873,18 +1821,23 @@ QBindingStorage::~QBindingStorage() QBindingStoragePrivate(d).destroy(); } +// ### Unused, retained for BC with 6.0 void QBindingStorage::maybeUpdateBindingAndRegister_helper(const QUntypedPropertyData *data) const +{ + registerDependency_helper(data); +} + +void QBindingStorage::registerDependency_helper(const QUntypedPropertyData *data) const { Q_ASSERT(bindingStatus); QUntypedPropertyData *dd = const_cast(data); auto storage = QBindingStoragePrivate(d).get(dd, /*create=*/ bindingStatus->currentlyEvaluatingBinding != nullptr); if (!storage) return; - if (auto *binding = storage->binding()) - binding->evaluateIfDirtyAndReturnTrueIfValueChanged(const_cast(data), bindingStatus); storage->registerWithCurrentlyEvaluatingBinding(bindingStatus->currentlyEvaluatingBinding); } + QPropertyBindingData *QBindingStorage::bindingData_helper(const QUntypedPropertyData *data) const { return QBindingStoragePrivate(d).get(data); diff --git a/src/corelib/kernel/qproperty.h b/src/corelib/kernel/qproperty.h index 717c6713d46..40b7a9452db 100644 --- a/src/corelib/kernel/qproperty.h +++ b/src/corelib/kernel/qproperty.h @@ -229,7 +229,7 @@ private: QtPrivate::QTagPreservingPointerToPointer prev; union { - QPropertyBindingPrivate *bindingToMarkDirty = nullptr; + QPropertyBindingPrivate *binding = nullptr; ChangeHandler changeHandler; QUntypedPropertyData *aliasedPropertyData; }; @@ -329,8 +329,6 @@ public: parameter_type value() const { - if (d.hasBinding()) - d.evaluateIfDirty(this); d.registerWithCurrentlyEvaluatingBinding(); return this->val; } @@ -389,9 +387,7 @@ public: QPropertyBinding setBinding(const QPropertyBinding &newBinding) { - QPropertyBinding oldBinding(d.setBinding(newBinding, this)); - notify(); - return oldBinding; + return QPropertyBinding(d.setBinding(newBinding, this)); } bool setBinding(const QUntypedPropertyBinding &newBinding) @@ -402,11 +398,6 @@ public: return true; } - void markDirty() { - d.markDirty(); - notify(); - } - #ifndef Q_CLANG_QDOC template QPropertyBinding setBinding(Functor &&f, @@ -852,11 +843,11 @@ public: bool isEmpty() { return !d; } - void maybeUpdateBindingAndRegister(const QUntypedPropertyData *data) const + void registerDependency(const QUntypedPropertyData *data) const { if (!d && !bindingStatus->currentlyEvaluatingBinding) return; - maybeUpdateBindingAndRegister_helper(data); + registerDependency_helper(data); } QtPrivate::QPropertyBindingData *bindingData(const QUntypedPropertyData *data) const { @@ -864,6 +855,9 @@ public: return nullptr; return bindingData_helper(data); } + // ### Qt 7: remove unused BIC shim + void maybeUpdateBindingAndRegister(const QUntypedPropertyData *data) const { registerDependency(data); } + QtPrivate::QPropertyBindingData *bindingData(QUntypedPropertyData *data, bool create) { if (!d && !create) @@ -871,6 +865,8 @@ public: return bindingData_helper(data, create); } private: + void registerDependency_helper(const QUntypedPropertyData *data) const; + // ### Unused, but keep for BC void maybeUpdateBindingAndRegister_helper(const QUntypedPropertyData *data) const; QtPrivate::QPropertyBindingData *bindingData_helper(const QUntypedPropertyData *data) const; QtPrivate::QPropertyBindingData *bindingData_helper(QUntypedPropertyData *data, bool create); @@ -923,7 +919,7 @@ public: parameter_type value() const { - qGetBindingStorage(owner())->maybeUpdateBindingAndRegister(this); + qGetBindingStorage(owner())->registerDependency(this); return this->val; } @@ -987,7 +983,6 @@ public: { QtPrivate::QPropertyBindingData *bd = qGetBindingStorage(owner())->bindingData(this, true); QUntypedPropertyBinding oldBinding(bd->setBinding(newBinding, this, HasSignal ? &signalCallBack : nullptr)); - notify(bd); return static_cast &>(oldBinding); } @@ -1018,15 +1013,6 @@ public: return bd && bd->binding() != nullptr; } - void markDirty() { - QBindingStorage *storage = qGetBindingStorage(owner()); - auto bd = storage->bindingData(this, /*create=*/false); - if (bd) { // if we have no BindingData, nobody can listen anyway - bd->markDirty(); - notify(bd); - } - } - QPropertyBinding binding() const { auto *bd = qGetBindingStorage(owner())->bindingData(this); @@ -1130,7 +1116,7 @@ public: parameter_type value() const { - qGetBindingStorage(owner())->maybeUpdateBindingAndRegister(this); + qGetBindingStorage(owner())->registerDependency(this); return (owner()->*Getter)(); } @@ -1176,15 +1162,13 @@ public: return *storage->bindingData(const_cast(this), true); } - void markDirty() { + void notify() { // computed property can't store a binding, so there's nothing to mark auto *storage = const_cast(qGetBindingStorage(owner())); auto bd = storage->bindingData(const_cast(this), false); if (bd) - bindingData().notifyObservers(this); + bd->notifyObservers(this); } - -private: }; #define Q_OBJECT_COMPUTED_PROPERTY(Class, Type, name, ...) \ diff --git a/src/corelib/kernel/qproperty_p.h b/src/corelib/kernel/qproperty_p.h index d4594335287..00978e8ea67 100644 --- a/src/corelib/kernel/qproperty_p.h +++ b/src/corelib/kernel/qproperty_p.h @@ -104,11 +104,11 @@ struct QPropertyObserverPointer void unlink(); - void setBindingToMarkDirty(QPropertyBindingPrivate *binding); + void setBindingToNotify(QPropertyBindingPrivate *binding); void setChangeHandler(QPropertyObserver::ChangeHandler changeHandler); void setAliasedProperty(QUntypedPropertyData *propertyPtr); - void notify(QPropertyBindingPrivate *triggeringBinding, QUntypedPropertyData *propertyDataPtr, bool knownToHaveChanged = false); + void notify(QUntypedPropertyData *propertyDataPtr); void observeProperty(QPropertyBindingDataPointer property); explicit operator bool() const { return ptr != nullptr; } @@ -172,14 +172,11 @@ private: private: - // a dependent property has changed, and the binding needs to be reevaluated on access - bool dirty = false; // used to detect binding loops for lazy evaluated properties bool updating = false; bool hasStaticObserver = false; bool hasBindingWrapper:1; // used to detect binding loops for eagerly evaluated properties - bool eagerlyUpdating:1; bool isQQmlPropertyBinding:1; const QtPrivate::BindingFunctionVTable *vtable; @@ -230,13 +227,11 @@ public: // public because the auto-tests access it, too. size_t dependencyObserverCount = 0; - bool isEagerlyUpdating() {return eagerlyUpdating;} - void setEagerlyUpdating(bool b) {eagerlyUpdating = b;} + bool isUpdating() {return updating;} QPropertyBindingPrivate(QMetaType metaType, const QtPrivate::BindingFunctionVTable *vtable, const QPropertyBindingSourceLocation &location, bool isQQmlPropertyBinding=false) : hasBindingWrapper(false) - , eagerlyUpdating(false) , isQQmlPropertyBinding(isQQmlPropertyBinding) , vtable(vtable) , location(location) @@ -245,7 +240,6 @@ public: ~QPropertyBindingPrivate(); - void setDirty(bool d) { dirty = d; } void setProperty(QUntypedPropertyData *propertyPtr) { propertyDataPtr = propertyPtr; } void setStaticObserver(QtPrivate::QPropertyObserverCallback callback, QtPrivate::QPropertyBindingWrapper bindingWrapper) { @@ -312,13 +306,7 @@ public: void unlinkAndDeref(); - void markDirtyAndNotifyObservers(); - bool evaluateIfDirtyAndReturnTrueIfValueChanged(const QUntypedPropertyData *data, QBindingStatus *status = nullptr) - { - if (!dirty) - return false; - return evaluateIfDirtyAndReturnTrueIfValueChanged_helper(data, status); - } + void evaluate(); static QPropertyBindingPrivate *get(const QUntypedPropertyBinding &binding) { return static_cast(binding.d.data()); } @@ -334,8 +322,6 @@ public: clearDependencyObservers(); } - bool requiresEagerEvaluation() const { return hasBindingWrapper; } - static QPropertyBindingPrivate *currentlyEvaluatingBinding(); bool hasCustomVTable() const @@ -353,9 +339,6 @@ public: delete[] reinterpret_cast(priv); } } - -private: - bool evaluateIfDirtyAndReturnTrueIfValueChanged_helper(const QUntypedPropertyData *data, QBindingStatus *status = nullptr); }; inline void QPropertyBindingDataPointer::setFirstObserver(QPropertyObserver *observer) @@ -434,7 +417,7 @@ public: const QBindingStorage *storage = qGetBindingStorage(owner()); // make sure we don't register this binding as a dependency to itself if (!inBindingWrapper(storage)) - storage->maybeUpdateBindingAndRegister(this); + storage->registerDependency(this); return this->val; } @@ -511,15 +494,6 @@ public: return bd && bd->binding() != nullptr; } - void markDirty() { - QBindingStorage *storage = qGetBindingStorage(owner()); - auto *bd = storage->bindingData(this, false); - if (bd) { - bd->markDirty(); - notify(bd); - } - } - void removeBindingUnlessInWrapper() { QBindingStorage *storage = qGetBindingStorage(owner()); @@ -576,6 +550,7 @@ public: auto *storage = const_cast(qGetBindingStorage(owner())); return *storage->bindingData(const_cast(this), true); } + private: void notify(const QtPrivate::QPropertyBindingData *binding) { diff --git a/src/corelib/kernel/qpropertyprivate.h b/src/corelib/kernel/qpropertyprivate.h index 896ad173953..b6d93529d1c 100644 --- a/src/corelib/kernel/qpropertyprivate.h +++ b/src/corelib/kernel/qpropertyprivate.h @@ -249,8 +249,7 @@ public: } - void evaluateIfDirty(const QUntypedPropertyData *property) const; - void markDirty(); + void evaluateIfDirty(const QUntypedPropertyData *) const; // ### Kept for BC reasons, unused void removeBinding() { diff --git a/src/corelib/kernel/qtimer.cpp b/src/corelib/kernel/qtimer.cpp index 333e6c24ba5..10e26fcdef6 100644 --- a/src/corelib/kernel/qtimer.cpp +++ b/src/corelib/kernel/qtimer.cpp @@ -241,7 +241,7 @@ void QTimer::start() stop(); d->nulltimer = (!d->inter && d->single); d->id = QObject::startTimer(d->inter, d->type); - d->isActiveData.markDirty(); + d->isActiveData.notify(); } /*! @@ -278,7 +278,7 @@ void QTimer::stop() if (d->id != INV_TIMER) { QObject::killTimer(d->id); d->id = INV_TIMER; - d->isActiveData.markDirty(); + d->isActiveData.notify(); } } @@ -763,9 +763,8 @@ void QTimer::setInterval(int msec) // No need to call markDirty() for d->isActiveData here, // as timer state actually does not change } - if (intervalChanged) - d->inter.markDirty(); + d->inter.notify(); } int QTimer::interval() const diff --git a/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp b/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp index 848226713c5..12fd124b18c 100644 --- a/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp +++ b/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp @@ -95,7 +95,6 @@ private slots: void noFakeDependencies(); void bindablePropertyWithInitialization(); - void markDirty(); }; void tst_QProperty::functorBinding() @@ -129,8 +128,8 @@ void tst_QProperty::multipleDependencies() QProperty sum; sum.setBinding([&]() { return firstDependency + secondDependency; }); - QCOMPARE(QPropertyBindingDataPointer::get(firstDependency).observerCount(), 0); - QCOMPARE(QPropertyBindingDataPointer::get(secondDependency).observerCount(), 0); + QCOMPARE(QPropertyBindingDataPointer::get(firstDependency).observerCount(), 1); + QCOMPARE(QPropertyBindingDataPointer::get(secondDependency).observerCount(), 1); QCOMPARE(sum.value(), int(3)); QCOMPARE(QPropertyBindingDataPointer::get(firstDependency).observerCount(), 1); @@ -484,23 +483,22 @@ class BindingLoopTester : public QObject void tst_QProperty::bindingLoop() { - QScopedPointer> firstProp; + QProperty firstProp; QProperty secondProp([&]() -> int { - return firstProp ? firstProp->value() : 0; + return firstProp.value(); }); QProperty thirdProp([&]() -> int { return secondProp.value(); }); - firstProp.reset(new QProperty()); - firstProp->setBinding([&]() -> int { - return secondProp.value(); + firstProp.setBinding([&]() -> int { + return secondProp.value() + thirdProp.value(); }); - QCOMPARE(thirdProp.value(), 0); - QCOMPARE(secondProp.binding().error().type(), QPropertyBindingError::BindingLoop); + thirdProp.setValue(10); + QCOMPARE(firstProp.binding().error().type(), QPropertyBindingError::BindingLoop); { @@ -1200,7 +1198,9 @@ void tst_QProperty::qobjectObservers() MyQObject object; int onValueChangedCalled = 0; { - auto handler = object.bindableFoo().onValueChanged([&onValueChangedCalled]() { ++onValueChangedCalled;}); + auto handler = object.bindableFoo().onValueChanged([&onValueChangedCalled]() { + ++onValueChangedCalled; + }); QCOMPARE(onValueChangedCalled, 0); object.setFoo(10); @@ -1606,82 +1606,6 @@ void tst_QProperty::bindablePropertyWithInitialization() QCOMPARE(tester.prop3().anotherValue, 20); } -class MarkDirtyTester : public QObject -{ - Q_OBJECT -public: - Q_PROPERTY(int value1 READ value1 WRITE setValue1 BINDABLE bindableValue1) - Q_PROPERTY(int value2 READ value2 WRITE setValue1 BINDABLE bindableValue2) - Q_PROPERTY(int computed READ computed BINDABLE bindableComputed) - - inline static int staticValue = 0; - - int value1() const {return m_value1;} - void setValue1(int val) {m_value1 = val;} - QBindable bindableValue1() {return { &m_value1 };} - - int value2() const {return m_value2;} - void setValue2(int val) { m_value2.setValue(val); } - QBindable bindableValue2() {return { &m_value2 };} - - int computed() const { return staticValue + m_value1; } - QBindable bindableComputed() {return {&m_computed};} - - void incrementStaticValue() { - ++staticValue; - m_computed.markDirty(); - } - - void markValue1Dirty() { - m_value1.markDirty(); - } - - void markValue2Dirty() { - m_value2.markDirty(); - } -private: - Q_OBJECT_BINDABLE_PROPERTY(MarkDirtyTester, int, m_value1, nullptr) - Q_OBJECT_COMPAT_PROPERTY(MarkDirtyTester, int, m_value2, &MarkDirtyTester::setValue2) - Q_OBJECT_COMPUTED_PROPERTY(MarkDirtyTester, int, m_computed, &MarkDirtyTester::computed) -}; - -void tst_QProperty::markDirty() -{ - { - QProperty testProperty; - int changeCounter = 0; - auto handler = testProperty.onValueChanged([&](){++changeCounter;}); - testProperty.markDirty(); - QCOMPARE(changeCounter, 1); - } - { - MarkDirtyTester dirtyTester; - int computedChangeCounter = 0; - int value1ChangeCounter = 0; - auto handler = dirtyTester.bindableComputed().onValueChanged([&](){ - computedChangeCounter++; - }); - auto handler2 = dirtyTester.bindableValue1().onValueChanged([&](){ - value1ChangeCounter++; - }); - dirtyTester.incrementStaticValue(); - QCOMPARE(computedChangeCounter, 1); - QCOMPARE(dirtyTester.computed(), 1); - dirtyTester.markValue1Dirty(); - QCOMPARE(value1ChangeCounter, 1); - QCOMPARE(computedChangeCounter, 1); - } - { - MarkDirtyTester dirtyTester; - int changeCounter = 0; - auto handler = dirtyTester.bindableValue2().onValueChanged([&](){ - changeCounter++; - }); - dirtyTester.markValue2Dirty(); - QCOMPARE(changeCounter, 1); - } -} - QTEST_MAIN(tst_QProperty); #include "tst_qproperty.moc" From 984bc7cc3e53e1a9003209858f89bd7fcbd5f81c Mon Sep 17 00:00:00 2001 From: Fabian Kosmale Date: Fri, 29 Jan 2021 13:29:10 +0100 Subject: [PATCH 12/14] Address thread safety issues in QProperty classes While we do not support cross-thread bindings, reading of properties from a different thread when no bindings are involved must continue to work. However the check whether bindings are involved used the QBindingStatus in the QObjectPrivate. That one contains the wrong value when the QObject is accessed from a different thread than the one it has affinity with. This patch reads from the thread_local directly instead to sidetstep the issue. Change-Id: I8ce2092f35e210566934e2439beb5d48fd8cf226 Reviewed-by: Fabian Kosmale Reviewed-by: Lars Knoll Reviewed-by: Qt CI Bot --- src/corelib/kernel/qproperty.cpp | 14 ++++++++++++-- src/corelib/kernel/qproperty.h | 2 +- src/corelib/kernel/qproperty_p.h | 10 +++++++--- 3 files changed, 20 insertions(+), 6 deletions(-) diff --git a/src/corelib/kernel/qproperty.cpp b/src/corelib/kernel/qproperty.cpp index ad254922832..1c642562a84 100644 --- a/src/corelib/kernel/qproperty.cpp +++ b/src/corelib/kernel/qproperty.cpp @@ -1830,11 +1830,14 @@ void QBindingStorage::maybeUpdateBindingAndRegister_helper(const QUntypedPropert void QBindingStorage::registerDependency_helper(const QUntypedPropertyData *data) const { Q_ASSERT(bindingStatus); + // Use ::bindingStatus to get the binding from TLS. This is required, so that reads from + // another thread do not register as dependencies + auto *currentBinding = QT_PREPEND_NAMESPACE(bindingStatus).currentlyEvaluatingBinding; QUntypedPropertyData *dd = const_cast(data); - auto storage = QBindingStoragePrivate(d).get(dd, /*create=*/ bindingStatus->currentlyEvaluatingBinding != nullptr); + auto storage = QBindingStoragePrivate(d).get(dd, /*create=*/ currentBinding != nullptr); if (!storage) return; - storage->registerWithCurrentlyEvaluatingBinding(bindingStatus->currentlyEvaluatingBinding); + storage->registerWithCurrentlyEvaluatingBinding(currentBinding); } @@ -1874,6 +1877,13 @@ bool isAnyBindingEvaluating() { return bindingStatus.currentlyEvaluatingBinding != nullptr; } + +bool isPropertyInBindingWrapper(const QUntypedPropertyData *property) +{ + return bindingStatus.currentCompatProperty && + bindingStatus.currentCompatProperty->property == property; +} + } // namespace QtPrivate end QT_END_NAMESPACE diff --git a/src/corelib/kernel/qproperty.h b/src/corelib/kernel/qproperty.h index 40b7a9452db..1c557fbacea 100644 --- a/src/corelib/kernel/qproperty.h +++ b/src/corelib/kernel/qproperty.h @@ -845,7 +845,7 @@ public: void registerDependency(const QUntypedPropertyData *data) const { - if (!d && !bindingStatus->currentlyEvaluatingBinding) + if (!bindingStatus->currentlyEvaluatingBinding) return; registerDependency_helper(data); } diff --git a/src/corelib/kernel/qproperty_p.h b/src/corelib/kernel/qproperty_p.h index 00978e8ea67..7ac02cb43d3 100644 --- a/src/corelib/kernel/qproperty_p.h +++ b/src/corelib/kernel/qproperty_p.h @@ -368,6 +368,10 @@ inline QPropertyObserverPointer QPropertyBindingDataPointer::firstObserver() con return { reinterpret_cast(ptr->d_ptr) }; } +namespace QtPrivate { + Q_CORE_EXPORT bool isPropertyInBindingWrapper(const QUntypedPropertyData *property); +} + template class QObjectCompatProperty : public QPropertyData { @@ -397,10 +401,10 @@ class QObjectCompatProperty : public QPropertyData (thisData->owner()->*Setter)(copy.valueBypassingBindings()); return true; } - inline bool inBindingWrapper(const QBindingStorage *storage) const + bool inBindingWrapper(const QBindingStorage *storage) const { - return storage->bindingStatus->currentCompatProperty && - storage->bindingStatus->currentCompatProperty->property == this; + return storage->bindingStatus->currentCompatProperty + && QtPrivate::isPropertyInBindingWrapper(this); } public: From bb44c18b673e2955592ee86774eb7cc25d390c17 Mon Sep 17 00:00:00 2001 From: Lars Knoll Date: Thu, 4 Feb 2021 17:20:33 +0100 Subject: [PATCH 13/14] Don't emit change notifications more often than required When a property value changes, first update all dependent bindings to their new value. Only once that is done send out all the notifications and changed signals. This way, if a property depends on multiple other properties, which all get changed, there will only be one notification; and (potentially invalid) intermediate values will not be observed. Fixes: QTBUG-89844 Change-Id: I086077934aee6dc940705f08a87bf8448708881f Reviewed-by: Lars Knoll Reviewed-by: Fabian Kosmale Reviewed-by: Andreas Buhr Reviewed-by: Andrei Golubev --- src/corelib/kernel/qproperty.cpp | 70 ++++++++++++++++--- src/corelib/kernel/qproperty_p.h | 10 ++- .../kernel/qproperty/tst_qproperty.cpp | 31 ++++++++ 3 files changed, 99 insertions(+), 12 deletions(-) diff --git a/src/corelib/kernel/qproperty.cpp b/src/corelib/kernel/qproperty.cpp index 1c642562a84..60f21d5b573 100644 --- a/src/corelib/kernel/qproperty.cpp +++ b/src/corelib/kernel/qproperty.cpp @@ -98,7 +98,7 @@ void QPropertyBindingPrivate::unlinkAndDeref() destroyAndFreeMemory(this); } -void QPropertyBindingPrivate::evaluate() +void QPropertyBindingPrivate::evaluateRecursive() { if (updating) { error = QPropertyBindingError(QPropertyBindingError::BindingLoop); @@ -121,23 +121,35 @@ void QPropertyBindingPrivate::evaluate() BindingEvaluationState evaluationFrame(this); - bool changed; auto bindingFunctor = reinterpret_cast(this) + QPropertyBindingPrivate::getSizeEnsuringAlignment(); if (hasBindingWrapper) { - changed = staticBindingWrapper(metaType, propertyDataPtr, + pendingNotify = staticBindingWrapper(metaType, propertyDataPtr, {vtable, bindingFunctor}); } else { - changed = vtable->call(metaType, propertyDataPtr, bindingFunctor); + pendingNotify = vtable->call(metaType, propertyDataPtr, bindingFunctor); } - if (!changed) + if (!pendingNotify || !firstObserver) return; - // notify dependent bindings and emit changed signal - if (firstObserver) + firstObserver.noSelfDependencies(this); + firstObserver.evaluateBindings(); +} + +void QPropertyBindingPrivate::notifyRecursive() +{ + if (!pendingNotify) + return; + pendingNotify = false; + Q_ASSERT(!updating); + updating = true; + if (firstObserver) { + firstObserver.noSelfDependencies(this); firstObserver.notify(propertyDataPtr); + } if (hasStaticObserver) staticObserverCallback(propertyDataPtr); + updating = false; } QUntypedPropertyBinding::QUntypedPropertyBinding() = default; @@ -248,7 +260,8 @@ QUntypedPropertyBinding QPropertyBindingData::setBinding(const QUntypedPropertyB newBindingRaw->prependObserver(observer); newBindingRaw->setStaticObserver(staticObserverCallback, guardCallback); - newBindingRaw->evaluate(); + newBindingRaw->evaluateRecursive(); + newBindingRaw->notifyRecursive(); } else if (observer) { d.setObservers(observer.ptr); } else { @@ -343,8 +356,10 @@ void QPropertyBindingData::registerWithCurrentlyEvaluatingBinding_helper(Binding void QPropertyBindingData::notifyObservers(QUntypedPropertyData *propertyDataPtr) const { QPropertyBindingDataPointer d{this}; - if (QPropertyObserverPointer observer = d.firstObserver()) + if (QPropertyObserverPointer observer = d.firstObserver()) { + observer.evaluateBindings(); observer.notify(propertyDataPtr); + } } int QPropertyBindingDataPointer::observerCount() const @@ -518,9 +533,9 @@ void QPropertyObserverPointer::notify(QUntypedPropertyData *propertyDataPtr) } case QPropertyObserver::ObserverNotifiesBinding: { - auto bindingToMarkDirty = observer->binding; + auto bindingToNotify = observer->binding; QPropertyObserverNodeProtector protector(observer); - bindingToMarkDirty->evaluate(); + bindingToNotify->notifyRecursive(); next = protector.next(); break; } @@ -534,6 +549,39 @@ void QPropertyObserverPointer::notify(QUntypedPropertyData *propertyDataPtr) } } +#ifndef QT_NO_DEBUG +void QPropertyObserverPointer::noSelfDependencies(QPropertyBindingPrivate *binding) +{ + auto observer = const_cast(ptr); + // See also comment in notify() + while (observer) { + if (QPropertyObserver::ObserverTag(observer->next.tag()) == QPropertyObserver::ObserverNotifiesBinding) + Q_ASSERT(observer->binding != binding); + + observer = observer->next.data(); + } + +} +#endif + +void QPropertyObserverPointer::evaluateBindings() +{ + auto observer = const_cast(ptr); + // See also comment in notify() + while (observer) { + QPropertyObserver *next = observer->next.data(); + + if (QPropertyObserver::ObserverTag(observer->next.tag()) == QPropertyObserver::ObserverNotifiesBinding) { + auto bindingToEvaluate = observer->binding; + QPropertyObserverNodeProtector protector(observer); + bindingToEvaluate->evaluateRecursive(); + next = protector.next(); + } + + observer = next; + } +} + void QPropertyObserverPointer::observeProperty(QPropertyBindingDataPointer property) { if (ptr->prev) diff --git a/src/corelib/kernel/qproperty_p.h b/src/corelib/kernel/qproperty_p.h index 7ac02cb43d3..3535d7e843b 100644 --- a/src/corelib/kernel/qproperty_p.h +++ b/src/corelib/kernel/qproperty_p.h @@ -109,6 +109,12 @@ struct QPropertyObserverPointer void setAliasedProperty(QUntypedPropertyData *propertyPtr); void notify(QUntypedPropertyData *propertyDataPtr); +#ifndef QT_NO_DEBUG + void noSelfDependencies(QPropertyBindingPrivate *binding); +#else + void noSelfDependencies(QPropertyBindingPrivate *) {} +#endif + void evaluateBindings(); void observeProperty(QPropertyBindingDataPointer property); explicit operator bool() const { return ptr != nullptr; } @@ -175,6 +181,7 @@ private: // used to detect binding loops for lazy evaluated properties bool updating = false; bool hasStaticObserver = false; + bool pendingNotify = false; bool hasBindingWrapper:1; // used to detect binding loops for eagerly evaluated properties bool isQQmlPropertyBinding:1; @@ -306,7 +313,8 @@ public: void unlinkAndDeref(); - void evaluate(); + void evaluateRecursive(); + void notifyRecursive(); static QPropertyBindingPrivate *get(const QUntypedPropertyBinding &binding) { return static_cast(binding.d.data()); } diff --git a/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp b/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp index 12fd124b18c..8cc4fe486a6 100644 --- a/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp +++ b/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp @@ -95,6 +95,7 @@ private slots: void noFakeDependencies(); void bindablePropertyWithInitialization(); + void noDoubleNotification(); }; void tst_QProperty::functorBinding() @@ -1606,6 +1607,36 @@ void tst_QProperty::bindablePropertyWithInitialization() QCOMPARE(tester.prop3().anotherValue, 20); } +void tst_QProperty::noDoubleNotification() +{ + /* dependency graph for this test + x --> y means y depends on x + a-->b-->d + \ ^ + \->c--/ + */ + QProperty a(0); + QProperty b; + b.setBinding([&](){ return a.value(); }); + QProperty c; + c.setBinding([&](){ return a.value(); }); + QProperty d; + d.setBinding([&](){ return b.value() + c.value(); }); + int nNotifications = 0; + int expected = 0; + auto connection = d.subscribe([&](){ + ++nNotifications; + QCOMPARE(d.value(), expected); + }); + QCOMPARE(nNotifications, 1); + expected = 2; + a = 1; + QCOMPARE(nNotifications, 2); + expected = 4; + a = 2; + QCOMPARE(nNotifications, 3); +} + QTEST_MAIN(tst_QProperty); #include "tst_qproperty.moc" From fdedcb6ec650236bef4a8c8f005b5dd24ef7d77a Mon Sep 17 00:00:00 2001 From: Lars Knoll Date: Fri, 5 Feb 2021 10:07:50 +0100 Subject: [PATCH 14/14] Add support for grouped property changes Add Qt::begin/endPropertyUpdateGroup() methods. These methods will group a set of property updates together and delay bindings evaluations or change notifications until the end of the update group. In cases where many properties get updated, this can avoid duplicated recalculations and change notifications. Change-Id: Ia78ae1d46abc6b7e5da5023442e081cb5c5ae67b Reviewed-by: Fabian Kosmale Reviewed-by: Andrei Golubev Reviewed-by: Andreas Buhr Reviewed-by: Lars Knoll --- src/corelib/kernel/qproperty.cpp | 235 ++++++++++++++++-- src/corelib/kernel/qproperty.h | 6 + src/corelib/kernel/qproperty_p.h | 42 ++-- src/corelib/kernel/qpropertyprivate.h | 60 ++++- .../kernel/qproperty/tst_qproperty.cpp | 77 +++++- 5 files changed, 372 insertions(+), 48 deletions(-) diff --git a/src/corelib/kernel/qproperty.cpp b/src/corelib/kernel/qproperty.cpp index 60f21d5b573..a5354532d2e 100644 --- a/src/corelib/kernel/qproperty.cpp +++ b/src/corelib/kernel/qproperty.cpp @@ -66,16 +66,17 @@ void QPropertyBindingPrivatePtr::reset(QtPrivate::RefCounted *ptr) noexcept void QPropertyBindingDataPointer::addObserver(QPropertyObserver *observer) { - if (auto *binding = bindingPtr()) { - observer->prev = &binding->firstObserver.ptr; - observer->next = binding->firstObserver.ptr; + if (auto *b = binding()) { + observer->prev = &b->firstObserver.ptr; + observer->next = b->firstObserver.ptr; if (observer->next) observer->next->prev = &observer->next; - binding->firstObserver.ptr = observer; + b->firstObserver.ptr = observer; } else { - Q_ASSERT(!(ptr->d_ptr & QPropertyBindingData::BindingBit)); - auto firstObserver = reinterpret_cast(ptr->d_ptr); - observer->prev = reinterpret_cast(&ptr->d_ptr); + auto &d = ptr->d_ref(); + Q_ASSERT(!(d & QPropertyBindingData::BindingBit)); + auto firstObserver = reinterpret_cast(d); + observer->prev = reinterpret_cast(&d); observer->next = firstObserver; if (observer->next) observer->next->prev = &observer->next; @@ -83,6 +84,182 @@ void QPropertyBindingDataPointer::addObserver(QPropertyObserver *observer) setFirstObserver(observer); } +/*! + \internal + + QPropertyDelayedNotifications is used to manage delayed notifications in grouped property updates. + It acts as a pool allocator for QPropertyProxyBindingData, and has methods to manage delayed + notifications. + + \sa beginPropertyUpdateGroup, endPropertyUpdateGroup + */ +struct QPropertyDelayedNotifications +{ + // we can't access the dynamic page size as we need a constant value + // use 4096 as a sensible default + static constexpr inline auto PageSize = 4096; + int ref = 0; + QPropertyDelayedNotifications *next = nullptr; // in case we have more than size dirty properties... + qsizetype used = 0; + // Size chosen to avoid allocating more than one page of memory, while still ensuring + // that we can store many delayed properties without doing further allocations + static constexpr qsizetype size = (PageSize - 3*sizeof(void *))/sizeof(QPropertyProxyBindingData); + QPropertyProxyBindingData delayedProperties[size]; + + /*! + \internal + This method is called when a property attempts to notify its observers while inside of a + property update group. Instead of actually notifying, it replaces \a bindingData's d_ptr + with a QPropertyProxyBindingData. + \a bindingData and \a propertyData are the binding data and property data of the property + whose notify call gets delayed. + \sa QPropertyBindingData::notifyObservers + */ + void addProperty(const QPropertyBindingData *bindingData, QUntypedPropertyData *propertyData) { + if (bindingData->isNotificationDelayed()) + return; + auto *data = this; + while (data->used == size) { + if (!data->next) + // add a new page + data->next = new QPropertyDelayedNotifications; + data = data->next; + } + auto *delayed = data->delayedProperties + data->used; + *delayed = QPropertyProxyBindingData { bindingData->d_ptr, bindingData, propertyData }; + ++data->used; + // preserve the binding bit for faster access + quintptr bindingBit = bindingData->d_ptr & QPropertyBindingData::BindingBit; + bindingData->d_ptr = reinterpret_cast(delayed) | QPropertyBindingData::DelayedNotificationBit | bindingBit; + Q_ASSERT(bindingData->d_ptr > 3); + if (!bindingBit) { + if (auto observer = reinterpret_cast(delayed->d_ptr)) + observer->prev = reinterpret_cast(&delayed->d_ptr); + } + } + + /*! + \internal + Called in Qt::endPropertyUpdateGroup. For the QPropertyProxyBindingData at position + \a index, it + \list + \li restores the original binding data that was modified in addProperty and + \li evaluates any bindings which depend on properties that were changed inside + the group. + \endlist + Change notifications are sent later with notify (following the logic of separating + binding updates and notifications used in non-deferred updates). + */ + void evaluateBindings(int index) { + auto *delayed = delayedProperties + index; + auto *bindingData = delayed->originalBindingData; + if (!bindingData) + return; + + bindingData->d_ptr = delayed->d_ptr; + Q_ASSERT(!(bindingData->d_ptr & QPropertyBindingData::DelayedNotificationBit)); + if (!bindingData->hasBinding()) { + if (auto observer = reinterpret_cast(bindingData->d_ptr)) + observer->prev = reinterpret_cast(&bindingData->d_ptr); + } + + QPropertyBindingDataPointer bindingDataPointer{bindingData}; + QPropertyObserverPointer observer = bindingDataPointer.firstObserver(); + if (observer) + observer.evaluateBindings(); + } + + /*! + \internal + Called in Qt::endPropertyUpdateGroup. For the QPropertyProxyBindingData at position + \a i, it + \list + \li resets the proxy binding data and + \li sends any pending notifications. + \endlist + */ + void notify(int index) { + auto *delayed = delayedProperties + index; + auto *bindingData = delayed->originalBindingData; + if (!bindingData) + return; + + delayed->originalBindingData = nullptr; + delayed->d_ptr = 0; + + QPropertyBindingDataPointer bindingDataPointer{bindingData}; + QPropertyObserverPointer observer = bindingDataPointer.firstObserver(); + if (observer) + observer.notify(delayed->propertyData); + } +}; + +static thread_local QPropertyDelayedNotifications *groupUpdateData = nullptr; + +/*! + \since 6.2 + + \relates template QProperty + + Marks the beginning of a property update group. Inside this group, + changing a property does neither immediately update any dependent properties + nor does it trigger change notifications. + Those are instead deferred until the group is ended by a call to endPropertyUpdateGroup. + + Groups can be nested. In that case, the deferral ends only after the outermost group has been + ended. + + \note Change notifications are only send after all property values affected by the group have + been updated to their new values. This allows re-establishing a class invariant if multiple + properties need to be updated, preventing any external observer from noticing an inconsistent + state. + + \sa Qt::endPropertyUpdateGroup + */ +void Qt::beginPropertyUpdateGroup() +{ + if (!groupUpdateData) + groupUpdateData = new QPropertyDelayedNotifications; + ++groupUpdateData->ref; +} + +/*! + \since 6.2 + \relates template QProperty + + Ends a property update group. If the outermost group has been ended, and deferred + binding evaluations and notifications happen now. + + \warning Calling endPropertyUpdateGroup without a preceding call to beginPropertyUpdateGroup + results in undefined behavior. + + \sa Qt::beginPropertyUpdateGroup + */ +void Qt::endPropertyUpdateGroup() +{ + auto *data = groupUpdateData; + Q_ASSERT(data->ref); + if (--data->ref) + return; + groupUpdateData = nullptr; + // update all delayed properties + auto start = data; + while (data) { + for (int i = 0; i < data->used; ++i) + data->evaluateBindings(i); + data = data->next; + } + // notify all delayed properties + data = start; + while (data) { + for (int i = 0; i < data->used; ++i) + data->notify(i); + auto *next = data->next; + delete data; + data = next; + } +} + QPropertyBindingPrivate::~QPropertyBindingPrivate() { if (firstObserver) @@ -123,13 +300,17 @@ void QPropertyBindingPrivate::evaluateRecursive() auto bindingFunctor = reinterpret_cast(this) + QPropertyBindingPrivate::getSizeEnsuringAlignment(); + bool changed = false; if (hasBindingWrapper) { - pendingNotify = staticBindingWrapper(metaType, propertyDataPtr, + changed = staticBindingWrapper(metaType, propertyDataPtr, {vtable, bindingFunctor}); } else { - pendingNotify = vtable->call(metaType, propertyDataPtr, bindingFunctor); + changed = vtable->call(metaType, propertyDataPtr, bindingFunctor); } - if (!pendingNotify || !firstObserver) + // If there was a change, we must set pendingNotify. + // If there was not, we must not clear it, as that only should happen in notifyRecursive + pendingNotify = pendingNotify || changed; + if (!changed || !firstObserver) return; firstObserver.noSelfDependencies(this); @@ -220,7 +401,7 @@ QPropertyBindingData::~QPropertyBindingData() observer.unlink(); observer = next; } - if (auto binding = d.bindingPtr()) + if (auto binding = d.binding()) binding->unlinkAndDeref(); } @@ -235,7 +416,8 @@ QUntypedPropertyBinding QPropertyBindingData::setBinding(const QUntypedPropertyB QPropertyBindingDataPointer d{this}; QPropertyObserverPointer observer; - if (auto *existingBinding = d.bindingPtr()) { + auto &data = d_ref(); + if (auto *existingBinding = d.binding()) { if (existingBinding == newBinding.data()) return QUntypedPropertyBinding(static_cast(oldBinding.data())); if (existingBinding->isUpdating()) { @@ -245,15 +427,15 @@ QUntypedPropertyBinding QPropertyBindingData::setBinding(const QUntypedPropertyB oldBinding = QPropertyBindingPrivatePtr(existingBinding); observer = static_cast(oldBinding.data())->takeObservers(); static_cast(oldBinding.data())->unlinkAndDeref(); - d_ptr = 0; + data = 0; } else { observer = d.firstObserver(); } if (newBinding) { newBinding.data()->addRef(); - d_ptr = reinterpret_cast(newBinding.data()); - d_ptr |= BindingBit; + data = reinterpret_cast(newBinding.data()); + data |= BindingBit; auto newBindingRaw = static_cast(newBinding.data()); newBindingRaw->setProperty(propertyDataPtr); if (observer) @@ -265,7 +447,7 @@ QUntypedPropertyBinding QPropertyBindingData::setBinding(const QUntypedPropertyB } else if (observer) { d.setObservers(observer.ptr); } else { - d_ptr &= ~QPropertyBindingData::BindingBit; + data = 0; } if (oldBinding) @@ -276,8 +458,7 @@ QUntypedPropertyBinding QPropertyBindingData::setBinding(const QUntypedPropertyB QPropertyBindingData::QPropertyBindingData(QPropertyBindingData &&other) : d_ptr(std::exchange(other.d_ptr, 0)) { - QPropertyBindingDataPointer d{this}; - d.fixupFirstObserverAfterMove(); + QPropertyBindingDataPointer::fixupAfterMove(this); } static thread_local QBindingStatus bindingStatus; @@ -325,11 +506,11 @@ void QPropertyBindingData::removeBinding_helper() { QPropertyBindingDataPointer d{this}; - auto *existingBinding = d.bindingPtr(); + auto *existingBinding = d.binding(); Q_ASSERT(existingBinding); auto observer = existingBinding->takeObservers(); - d_ptr = 0; + d_ref() = 0; if (observer) d.setObservers(observer.ptr); existingBinding->unlinkAndDeref(); @@ -355,11 +536,19 @@ void QPropertyBindingData::registerWithCurrentlyEvaluatingBinding_helper(Binding void QPropertyBindingData::notifyObservers(QUntypedPropertyData *propertyDataPtr) const { + if (isNotificationDelayed()) + return; QPropertyBindingDataPointer d{this}; - if (QPropertyObserverPointer observer = d.firstObserver()) { - observer.evaluateBindings(); - observer.notify(propertyDataPtr); + QPropertyObserverPointer observer = d.firstObserver(); + if (!observer) + return; + auto *delay = groupUpdateData; + if (delay) { + delay->addProperty(this, propertyDataPtr); + return; } + observer.evaluateBindings(); + observer.notify(propertyDataPtr); } int QPropertyBindingDataPointer::observerCount() const diff --git a/src/corelib/kernel/qproperty.h b/src/corelib/kernel/qproperty.h index 1c557fbacea..a6cb7ada482 100644 --- a/src/corelib/kernel/qproperty.h +++ b/src/corelib/kernel/qproperty.h @@ -63,6 +63,11 @@ QT_BEGIN_NAMESPACE +namespace Qt { +Q_CORE_EXPORT void beginPropertyUpdateGroup(); +Q_CORE_EXPORT void endPropertyUpdateGroup(); +} + template class QPropertyData : public QUntypedPropertyData { @@ -217,6 +222,7 @@ protected: using ChangeHandler = void (*)(QPropertyObserver*, QUntypedPropertyData *); private: + friend struct QPropertyDelayedNotifications; friend struct QPropertyObserverNodeProtector; friend class QPropertyObserver; friend struct QPropertyObserverPointer; diff --git a/src/corelib/kernel/qproperty_p.h b/src/corelib/kernel/qproperty_p.h index 3535d7e843b..58a000e9bf0 100644 --- a/src/corelib/kernel/qproperty_p.h +++ b/src/corelib/kernel/qproperty_p.h @@ -71,19 +71,18 @@ struct Q_AUTOTEST_EXPORT QPropertyBindingDataPointer { const QtPrivate::QPropertyBindingData *ptr = nullptr; - QPropertyBindingPrivate *bindingPtr() const + QPropertyBindingPrivate *binding() const { - if (ptr->d_ptr & QtPrivate::QPropertyBindingData::BindingBit) - return reinterpret_cast(ptr->d_ptr - QtPrivate::QPropertyBindingData::BindingBit); - return nullptr; + return ptr->binding(); } void setObservers(QPropertyObserver *observer) { - observer->prev = reinterpret_cast(&(ptr->d_ptr)); - ptr->d_ptr = reinterpret_cast(observer); + auto &d = ptr->d_ref(); + observer->prev = reinterpret_cast(&d); + d = reinterpret_cast(observer); } - void fixupFirstObserverAfterMove() const; + static void fixupAfterMove(QtPrivate::QPropertyBindingData *ptr); void addObserver(QPropertyObserver *observer); void setFirstObserver(QPropertyObserver *observer); QPropertyObserverPointer firstObserver() const; @@ -351,29 +350,36 @@ public: inline void QPropertyBindingDataPointer::setFirstObserver(QPropertyObserver *observer) { - if (auto *binding = bindingPtr()) { - binding->firstObserver.ptr = observer; + if (auto *b = binding()) { + b->firstObserver.ptr = observer; return; } - ptr->d_ptr = reinterpret_cast(observer); + auto &d = ptr->d_ref(); + d = reinterpret_cast(observer); } -inline void QPropertyBindingDataPointer::fixupFirstObserverAfterMove() const +inline void QPropertyBindingDataPointer::fixupAfterMove(QtPrivate::QPropertyBindingData *ptr) { + auto &d = ptr->d_ref(); + if (ptr->isNotificationDelayed()) { + QPropertyProxyBindingData *proxyData + = reinterpret_cast(d & ~QtPrivate::QPropertyBindingData::BindingBit); + proxyData->originalBindingData = ptr; + } // If QPropertyBindingData has been moved, and it has an observer - // we have to adjust the firstObesrver's prev pointer to point to + // we have to adjust the firstObserver's prev pointer to point to // the moved to QPropertyBindingData's d_ptr - if (ptr->d_ptr & QtPrivate::QPropertyBindingData::BindingBit) + if (d & QtPrivate::QPropertyBindingData::BindingBit) return; // nothing to do if the observer is stored in the binding - if (auto observer = firstObserver()) - observer.ptr->prev = reinterpret_cast(&(ptr->d_ptr)); + if (auto observer = reinterpret_cast(d)) + observer->prev = reinterpret_cast(&d); } inline QPropertyObserverPointer QPropertyBindingDataPointer::firstObserver() const { - if (auto *binding = bindingPtr()) - return binding->firstObserver; - return { reinterpret_cast(ptr->d_ptr) }; + if (auto *b = binding()) + return b->firstObserver; + return { reinterpret_cast(ptr->d()) }; } namespace QtPrivate { diff --git a/src/corelib/kernel/qpropertyprivate.h b/src/corelib/kernel/qpropertyprivate.h index b6d93529d1c..f59221c4dfb 100644 --- a/src/corelib/kernel/qpropertyprivate.h +++ b/src/corelib/kernel/qpropertyprivate.h @@ -143,7 +143,6 @@ private: QtPrivate::RefCounted *d; }; - class QUntypedPropertyBinding; class QPropertyBindingPrivate; struct QPropertyBindingDataPointer; @@ -158,6 +157,24 @@ public: template class QPropertyData; +// Used for grouped property evaluations +namespace QtPrivate { +class QPropertyBindingData; +} +struct QPropertyDelayedNotifications; +struct QPropertyProxyBindingData +{ + // acts as QPropertyBindingData::d_ptr + quintptr d_ptr; + /* + The two members below store the original binding data and property + data pointer of the property which gets proxied. + They are set in QPropertyDelayedNotifications::addProperty + */ + const QtPrivate::QPropertyBindingData *originalBindingData; + QUntypedPropertyData *propertyData; +}; + namespace QtPrivate { struct BindingEvaluationState; @@ -218,6 +235,16 @@ struct QPropertyBindingFunction { using QPropertyObserverCallback = void (*)(QUntypedPropertyData *); using QPropertyBindingWrapper = bool(*)(QMetaType, QUntypedPropertyData *dataPtr, QPropertyBindingFunction); +/*! + \internal + A property normally consists of the actual property value and metadata for the binding system. + QPropertyBindingData is the latter part. It stores a pointer to either + - a (potentially empty) linked list of notifiers, in case there is no binding set, + - an actual QUntypedPropertyBinding when the property has a binding, + - or a pointer to QPropertyProxyBindingData when notifications occur inside a grouped update. + + \sa QPropertyDelayedNotifications, beginPropertyUpdateGroup + */ class Q_CORE_EXPORT QPropertyBindingData { // Mutable because the address of the observer of the currently evaluating binding is stored here, for @@ -225,6 +252,7 @@ class Q_CORE_EXPORT QPropertyBindingData mutable quintptr d_ptr = 0; friend struct QT_PREPEND_NAMESPACE(QPropertyBindingDataPointer); friend class QT_PREPEND_NAMESPACE(QQmlPropertyBinding); + friend struct QT_PREPEND_NAMESPACE(QPropertyDelayedNotifications); Q_DISABLE_COPY(QPropertyBindingData) public: QPropertyBindingData() = default; @@ -232,9 +260,13 @@ public: QPropertyBindingData &operator=(QPropertyBindingData &&other) = delete; ~QPropertyBindingData(); - static inline constexpr quintptr BindingBit = 0x1; // Is d_ptr pointing to a binding (1) or list of notifiers (0)? + // Is d_ptr pointing to a binding (1) or list of notifiers (0)? + static inline constexpr quintptr BindingBit = 0x1; + // Is d_ptr pointing to QPropertyProxyBindingData (1) or to an actual binding/list of notifiers? + static inline constexpr quintptr DelayedNotificationBit = 0x2; bool hasBinding() const { return d_ptr & BindingBit; } + bool isNotificationDelayed() const { return d_ptr & DelayedNotificationBit; } QUntypedPropertyBinding setBinding(const QUntypedPropertyBinding &newBinding, QUntypedPropertyData *propertyDataPtr, @@ -243,8 +275,9 @@ public: QPropertyBindingPrivate *binding() const { - if (d_ptr & BindingBit) - return reinterpret_cast(d_ptr - BindingBit); + quintptr dd = d(); + if (dd & BindingBit) + return reinterpret_cast(dd - BindingBit); return nullptr; } @@ -266,6 +299,25 @@ public: void registerWithCurrentlyEvaluatingBinding() const; void notifyObservers(QUntypedPropertyData *propertyDataPtr) const; private: + /*! + \internal + Returns a reference to d_ptr, except when d_ptr points to a proxy. + In that case, a reference to proxy->d_ptr is returned instead. + + To properly support proxying, direct access to d_ptr only occcurs when + - a function actually deals with proxying (e.g. + QPropertyDelayedNotifications::addProperty), + - only the tag value is accessed (e.g. hasBinding) or + - inside a constructor. + */ + quintptr &d_ref() const + { + quintptr &d = d_ptr; + if (isNotificationDelayed()) + return reinterpret_cast(d_ptr & ~(BindingBit|DelayedNotificationBit))->d_ptr; + return d; + } + quintptr d() const { return d_ref(); } void registerWithCurrentlyEvaluatingBinding_helper(BindingEvaluationState *currentBinding) const; void removeBinding_helper(); }; diff --git a/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp b/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp index 8cc4fe486a6..0b05353bf08 100644 --- a/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp +++ b/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp @@ -96,6 +96,8 @@ private slots: void bindablePropertyWithInitialization(); void noDoubleNotification(); + void groupedNotifications(); + void groupedNotificationConsistency(); }; void tst_QProperty::functorBinding() @@ -260,12 +262,12 @@ void tst_QProperty::avoidDependencyAllocationAfterFirstEval() QCOMPARE(propWithBinding.value(), int(11)); - QVERIFY(QPropertyBindingDataPointer::get(propWithBinding).bindingPtr()); - QCOMPARE(QPropertyBindingDataPointer::get(propWithBinding).bindingPtr()->dependencyObserverCount, 2u); + QVERIFY(QPropertyBindingDataPointer::get(propWithBinding).binding()); + QCOMPARE(QPropertyBindingDataPointer::get(propWithBinding).binding()->dependencyObserverCount, 2u); firstDependency = 100; QCOMPARE(propWithBinding.value(), int(110)); - QCOMPARE(QPropertyBindingDataPointer::get(propWithBinding).bindingPtr()->dependencyObserverCount, 2u); + QCOMPARE(QPropertyBindingDataPointer::get(propWithBinding).binding()->dependencyObserverCount, 2u); } void tst_QProperty::boolProperty() @@ -1637,6 +1639,75 @@ void tst_QProperty::noDoubleNotification() QCOMPARE(nNotifications, 3); } +void tst_QProperty::groupedNotifications() +{ + QProperty a(0); + QProperty b; + b.setBinding([&](){ return a.value(); }); + QProperty c; + c.setBinding([&](){ return a.value(); }); + QProperty d; + QProperty e; + e.setBinding([&](){ return b.value() + c.value() + d.value(); }); + int nNotifications = 0; + int expected = 0; + auto connection = e.subscribe([&](){ + ++nNotifications; + QCOMPARE(e.value(), expected); + }); + QCOMPARE(nNotifications, 1); + + expected = 2; + Qt::beginPropertyUpdateGroup(); + a = 1; + QCOMPARE(b.value(), 0); + QCOMPARE(c.value(), 0); + QCOMPARE(d.value(), 0); + QCOMPARE(nNotifications, 1); + Qt::endPropertyUpdateGroup(); + QCOMPARE(b.value(), 1); + QCOMPARE(c.value(), 1); + QCOMPARE(e.value(), 2); + QCOMPARE(nNotifications, 2); + + expected = 7; + Qt::beginPropertyUpdateGroup(); + a = 2; + d = 3; + QCOMPARE(b.value(), 1); + QCOMPARE(c.value(), 1); + QCOMPARE(d.value(), 3); + QCOMPARE(nNotifications, 2); + Qt::endPropertyUpdateGroup(); + QCOMPARE(b.value(), 2); + QCOMPARE(c.value(), 2); + QCOMPARE(e.value(), 7); + QCOMPARE(nNotifications, 3); + + +} + +void tst_QProperty::groupedNotificationConsistency() +{ + QProperty i(0); + QProperty j(0); + bool areEqual = true; + + auto observer = i.onValueChanged([&](){ + areEqual = i == j; + }); + + i = 1; + j = 1; + QVERIFY(!areEqual); // value changed runs before j = 1 + + Qt::beginPropertyUpdateGroup(); + i = 2; + j = 2; + Qt::endPropertyUpdateGroup(); + QVERIFY(areEqual); // value changed runs after everything has been evaluated +} + QTEST_MAIN(tst_QProperty); #include "tst_qproperty.moc"