From 14d1108d3562f6e409505ba49b7bce82d70bd64f Mon Sep 17 00:00:00 2001 From: Thiago Macieira Date: Thu, 10 Aug 2023 21:38:23 -0700 Subject: [PATCH] Deprecate Q_ASSUME() We've known for a long time that this is producing worse code with GCC because of how we implemented in Q_ASSUME_IMPL(). So bite the bullet and actually deprecate the macro, replacing all extant Q_ASSUME() with Q_ASSERT(). The replacement is in C++23. Backporting the support onto Q_ASSUME_IMPL was previously rejected by reviewers. [ChangeLog][Deprecation Notice] The Q_ASSUME() macro is deprecated. This macro has different side-effects depending on the compiler used (GCC compared to Clang and MSVC), and there are certain conditions under which GCC is known to produce worse code than if the macro was absent. To give a hint to the compiler for optimizations, use the C++23 [[assume]] attribute. Change-Id: I80612a7d275c41f1baf0fffd177a3a4ad819fb2d Reviewed-by: Volker Hilsheimer --- src/corelib/global/qassert.h | 5 ++++- src/corelib/io/qbuffer.cpp | 2 +- src/corelib/kernel/qvariant.cpp | 2 +- src/corelib/serialization/qjsonvalue.cpp | 2 +- src/corelib/text/qbytearray.cpp | 2 +- src/corelib/text/qlocale_tools.cpp | 2 +- src/corelib/text/qregularexpression.cpp | 2 +- src/corelib/text/qstring.cpp | 2 +- src/corelib/text/qstringconverter.cpp | 2 +- src/corelib/tools/qcryptographichash.cpp | 6 +++--- src/corelib/tools/qvarlengtharray.h | 2 +- src/gui/painting/qpaintengine_raster.cpp | 2 +- src/gui/painting/qpaintengineex.cpp | 4 ++-- src/network/kernel/qnetworkdatagram.cpp | 2 +- src/plugins/platformthemes/gtk3/qgtk3interface.cpp | 4 ++-- tests/auto/gui/kernel/qwindow/tst_qwindow.cpp | 4 ++-- 16 files changed, 24 insertions(+), 21 deletions(-) diff --git a/src/corelib/global/qassert.h b/src/corelib/global/qassert.h index 862a707e75..0dc201c53c 100644 --- a/src/corelib/global/qassert.h +++ b/src/corelib/global/qassert.h @@ -79,11 +79,14 @@ inline T *q_check_ptr(T *p) { Q_CHECK_PTR(p); return p; } # endif #endif +Q_DECL_DEPRECATED_X("Q_ASSUME() is deprecated because it can produce worse code than when it's absent; " + "use C++23 [[assume]] instead") +inline bool qt_assume_is_deprecateed(bool cond) noexcept { return cond; } #define Q_ASSUME(Expr) \ [] (bool valueOfExpression) {\ Q_ASSERT_X(valueOfExpression, "Q_ASSUME()", "Assumption in Q_ASSUME(\"" #Expr "\") was not correct");\ Q_ASSUME_IMPL(valueOfExpression);\ - }(Expr) + }(qt_assume_is_deprecated(Expr)) // Don't use these in C++ mode, use static_assert directly. // These are here only to keep old code compiling. diff --git a/src/corelib/io/qbuffer.cpp b/src/corelib/io/qbuffer.cpp index ba1ce7463b..763620692c 100644 --- a/src/corelib/io/qbuffer.cpp +++ b/src/corelib/io/qbuffer.cpp @@ -401,7 +401,7 @@ qint64 QBuffer::writeData(const char *data, qint64 len) if (required > quint64(d->buf->size())) { // capacity exceeded // The following must hold, since qsizetype covers half the virtual address space: - Q_ASSUME(required <= quint64((std::numeric_limits::max)())); + Q_ASSERT(required <= quint64((std::numeric_limits::max)())); d->buf->resize(qsizetype(required)); if (quint64(d->buf->size()) != required) { // could not resize qWarning("QBuffer::writeData: Memory allocation error"); diff --git a/src/corelib/kernel/qvariant.cpp b/src/corelib/kernel/qvariant.cpp index bb61f771b6..2935002809 100644 --- a/src/corelib/kernel/qvariant.cpp +++ b/src/corelib/kernel/qvariant.cpp @@ -276,7 +276,7 @@ static void customConstruct(const QtPrivate::QMetaTypeInterface *iface, QVariant if constexpr (moveOption == ForceMove) Q_ASSERT(isMoveConstructible(iface)); if constexpr (nullability == NonNull) - Q_ASSUME(copy != nullptr); + Q_ASSERT(copy != nullptr); // need to check for nullptr_t here, as this can get called by fromValue(nullptr). fromValue() uses // std::addressof(value) which in this case returns the address of the nullptr object. diff --git a/src/corelib/serialization/qjsonvalue.cpp b/src/corelib/serialization/qjsonvalue.cpp index 1cc8e71d9b..f21faa25f6 100644 --- a/src/corelib/serialization/qjsonvalue.cpp +++ b/src/corelib/serialization/qjsonvalue.cpp @@ -1011,7 +1011,7 @@ QJsonValue QJsonValueConstRef::concrete(QJsonValueConstRef self) noexcept QString QJsonValueConstRef::objectKey(QJsonValueConstRef self) { Q_ASSERT(self.is_object); - Q_ASSUME(self.is_object); + Q_ASSERT(self.is_object); const QCborContainerPrivate *d = QJsonPrivate::Value::container(self); qsizetype index = QJsonPrivate::Value::indexHelper(self); diff --git a/src/corelib/text/qbytearray.cpp b/src/corelib/text/qbytearray.cpp index 4628c55ce5..f53a540703 100644 --- a/src/corelib/text/qbytearray.cpp +++ b/src/corelib/text/qbytearray.cpp @@ -276,7 +276,7 @@ int qstricmp(const char *str1, const char *str2) // yes, find out where uint start = qCountTrailingZeroBits(mask); uint end = sizeof(mask) * 8 - qCountLeadingZeroBits(mask); - Q_ASSUME(end >= start); + Q_ASSERT(end >= start); offset += start; n = end - start; break; diff --git a/src/corelib/text/qlocale_tools.cpp b/src/corelib/text/qlocale_tools.cpp index 0402203c27..dea26e2777 100644 --- a/src/corelib/text/qlocale_tools.cpp +++ b/src/corelib/text/qlocale_tools.cpp @@ -729,7 +729,7 @@ static T dtoString(double d, QLocaleData::DoubleForm form, int precision, bool u result.append(Char(uppercase ? 'E' : 'e')); result.append(Char(exponent < 0 ? '-' : '+')); exponent = std::abs(exponent); - Q_ASSUME(exponent <= D::max_exponent10 + D::max_digits10); + Q_ASSERT(exponent <= D::max_exponent10 + D::max_digits10); int exponentDigits = digits(exponent); // C's printf guarantees a two-digit exponent, and so do we: if (exponentDigits == 1) diff --git a/src/corelib/text/qregularexpression.cpp b/src/corelib/text/qregularexpression.cpp index 93e4f7d1f9..d5e08d825a 100644 --- a/src/corelib/text/qregularexpression.cpp +++ b/src/corelib/text/qregularexpression.cpp @@ -1104,7 +1104,7 @@ void QRegularExpressionPrivate::doMatch(QRegularExpressionMatchPrivate *priv, const QRegularExpressionMatchPrivate *previous) const { Q_ASSERT(priv); - Q_ASSUME(priv != previous); + Q_ASSERT(priv != previous); const qsizetype subjectLength = priv->subject.size(); diff --git a/src/corelib/text/qstring.cpp b/src/corelib/text/qstring.cpp index d70ae5c7f4..f20eee89fc 100644 --- a/src/corelib/text/qstring.cpp +++ b/src/corelib/text/qstring.cpp @@ -9755,7 +9755,7 @@ qsizetype QtPrivate::findString(QLatin1StringView haystack, qsizetype from, QLat if (cs == Qt::CaseSensitive) { if (needle.size() == 1) { - Q_ASSUME(haystack.data() != nullptr); // see size check above + Q_ASSERT(haystack.data() != nullptr); // see size check above if (auto it = memchr(haystack.data() + from, needle.front().toLatin1(), adjustedSize)) return static_cast(it) - haystack.data(); return -1; diff --git a/src/corelib/text/qstringconverter.cpp b/src/corelib/text/qstringconverter.cpp index 8857433544..075f39b07d 100644 --- a/src/corelib/text/qstringconverter.cpp +++ b/src/corelib/text/qstringconverter.cpp @@ -203,7 +203,7 @@ static inline const uchar *simdFindNonAscii(const uchar *src, const uchar *end, continue; uint n = _mm256_movemask_epi8(data); - Q_ASSUME(n); + Q_ASSERT(n); // find the next probable ASCII character // we don't want to load 32 bytes again in this loop if we know there are non-ASCII diff --git a/src/corelib/tools/qcryptographichash.cpp b/src/corelib/tools/qcryptographichash.cpp index d71c822ef7..adef9198c5 100644 --- a/src/corelib/tools/qcryptographichash.cpp +++ b/src/corelib/tools/qcryptographichash.cpp @@ -1289,9 +1289,9 @@ using HashBlock = QSmallByteArray; static HashBlock xored(const HashBlock &block, quint8 val) noexcept { // some hints for the optimizer: - Q_ASSUME(block.size() >= minHashBlockSize()); - Q_ASSUME(block.size() <= maxHashBlockSize()); - Q_ASSUME(block.size() % gcdHashBlockSize() == 0); + Q_ASSERT(block.size() >= minHashBlockSize()); + Q_ASSERT(block.size() <= maxHashBlockSize()); + Q_ASSERT(block.size() % gcdHashBlockSize() == 0); HashBlock result; result.resizeForOverwrite(block.size()); for (qsizetype i = 0; i < block.size(); ++i) diff --git a/src/corelib/tools/qvarlengtharray.h b/src/corelib/tools/qvarlengtharray.h index 1759fba415..db04b683b1 100644 --- a/src/corelib/tools/qvarlengtharray.h +++ b/src/corelib/tools/qvarlengtharray.h @@ -823,7 +823,7 @@ Q_OUTOFLINE_TEMPLATE void QVLABase::reallocate_impl(qsizetype prealloc, void qsizetype osize = size(); const qsizetype copySize = qMin(asize, osize); - Q_ASSUME(copySize >= 0); + Q_ASSERT(copySize >= 0); if (aalloc != capacity()) { QVLABaseBase::malloced_ptr guard; diff --git a/src/gui/painting/qpaintengine_raster.cpp b/src/gui/painting/qpaintengine_raster.cpp index a50f5aec60..4d730fccc3 100644 --- a/src/gui/painting/qpaintengine_raster.cpp +++ b/src/gui/painting/qpaintengine_raster.cpp @@ -2261,7 +2261,7 @@ void QRasterPaintEngine::drawImage(const QRectF &r, const QImage &img, const QRe || d->rasterBuffer->compositionMode == QPainter::CompositionMode_Source)) { RotationType rotationType = qRotationType(s->matrix); - Q_ASSUME(d->rasterBuffer->format < QImage::NImageFormats); + Q_ASSERT(d->rasterBuffer->format < QImage::NImageFormats); const QPixelLayout::BPP plBpp = qPixelLayouts[d->rasterBuffer->format].bpp; if (rotationType != NoRotation && qMemRotateFunctions[plBpp][rotationType] && img.rect().contains(sr.toAlignedRect())) { diff --git a/src/gui/painting/qpaintengineex.cpp b/src/gui/painting/qpaintengineex.cpp index 7f1c7e356d..9468876c23 100644 --- a/src/gui/painting/qpaintengineex.cpp +++ b/src/gui/painting/qpaintengineex.cpp @@ -902,7 +902,7 @@ void QPaintEngineEx::drawPoints(const QPoint *points, int pointCount) void QPaintEngineEx::drawPolygon(const QPointF *points, int pointCount, PolygonDrawMode mode) { - Q_ASSUME(pointCount >= 2); + Q_ASSERT(pointCount >= 2); QVectorPath path((const qreal *) points, pointCount, nullptr, QVectorPath::polygonFlags(mode)); if (mode == PolylineMode) @@ -913,7 +913,7 @@ void QPaintEngineEx::drawPolygon(const QPointF *points, int pointCount, PolygonD void QPaintEngineEx::drawPolygon(const QPoint *points, int pointCount, PolygonDrawMode mode) { - Q_ASSUME(pointCount >= 2); + Q_ASSERT(pointCount >= 2); int count = pointCount<<1; QVarLengthArray pts(count); diff --git a/src/network/kernel/qnetworkdatagram.cpp b/src/network/kernel/qnetworkdatagram.cpp index e9e3a67edf..a97eb25a51 100644 --- a/src/network/kernel/qnetworkdatagram.cpp +++ b/src/network/kernel/qnetworkdatagram.cpp @@ -481,7 +481,7 @@ void QNetworkDatagram::makeReply_helper_inplace(const QByteArray &data) void QNetworkDatagram::destroy(QNetworkDatagramPrivate *d) { - Q_ASSUME(d); + Q_ASSERT(d); delete d; } diff --git a/src/plugins/platformthemes/gtk3/qgtk3interface.cpp b/src/plugins/platformthemes/gtk3/qgtk3interface.cpp index 5229cb7346..33d0182521 100644 --- a/src/plugins/platformthemes/gtk3/qgtk3interface.cpp +++ b/src/plugins/platformthemes/gtk3/qgtk3interface.cpp @@ -289,8 +289,8 @@ QImage QGtk3Interface::qt_convert_gdk_pixbuf(GdkPixbuf *buf) const const guint8 *gdata = gdk_pixbuf_read_pixels(buf); static_assert(std::is_same::value, "guint8 has diverted from uchar. Code needs fixing."); - Q_ASSUME(gdk_pixbuf_get_bits_per_sample(buf) == 8); - Q_ASSUME(gdk_pixbuf_get_n_channels(buf) == 4); + Q_ASSERT(gdk_pixbuf_get_bits_per_sample(buf) == 8); + Q_ASSERT(gdk_pixbuf_get_n_channels(buf) == 4); const uchar *data = static_cast(gdata); const int width = gdk_pixbuf_get_width(buf); diff --git a/tests/auto/gui/kernel/qwindow/tst_qwindow.cpp b/tests/auto/gui/kernel/qwindow/tst_qwindow.cpp index 2c82d99251..2cc94f04e5 100644 --- a/tests/auto/gui/kernel/qwindow/tst_qwindow.cpp +++ b/tests/auto/gui/kernel/qwindow/tst_qwindow.cpp @@ -2803,11 +2803,11 @@ void tst_QWindow::stateChangeSignal() "On other operating systems, the signal may be emitted twice."); #endif QWindow w; - Q_ASSUME(connect (&w, &QWindow::windowStateChanged, [](Qt::WindowState s){qCDebug(lcTests) << "State change to" << s;})); + Q_ASSERT(connect (&w, &QWindow::windowStateChanged, [](Qt::WindowState s){qCDebug(lcTests) << "State change to" << s;})); QSignalSpy spy(&w, SIGNAL(windowStateChanged(Qt::WindowState))); unsigned short signalCount = 0; QList effectiveStates; - Q_ASSUME(connect(&w, &QWindow::windowStateChanged, [&effectiveStates](Qt::WindowState state) + Q_ASSERT(connect(&w, &QWindow::windowStateChanged, [&effectiveStates](Qt::WindowState state) { effectiveStates.append(state); })); // Part 1: // => test signal emission on programmatic state changes