From c5608c675a61a413b4a32c6c99721a1bb2f41e71 Mon Sep 17 00:00:00 2001 From: Giuseppe D'Angelo Date: Wed, 5 Feb 2025 18:33:25 +0100 Subject: [PATCH] Geometry classes: honor the documentation when it talks about rounding The floating-point based geometry classes promise rounding in their documentation, in at least two places. 1) when fp math is applied to an integer-based class, the result is the same integer class. For instance, QPoint(1, 2) * 3.14 yields a QPoint (!). The documentation says that the result is rounded to the closest integer. 2) when a fp-based class is converted to the corresponding integer-based one (e.g. QRectF::toRect()). These functions have the same notice. Insofar qRound has been used in the implementation of these functions. There's a problem: qRound does not actually honor the "round to closest integer" contract. Specifically, if the input floating point is out of range for the destination type, then the behavior is undefined (cf. a47a5932a64a8867077cab7c8efa57587b637481). Since we have reports of cases where out-of-range values are indeed passed through these functions, I'm extremely wary of changing the behavior to be UB. Instead, I'm making these functions actually do what the documentation says they do. To this end, refactor the various qRound overloads and split the rounding part from the casting to the destination type. The rounding is kept as private API, using the existing arch-specific implementations. The casting is generic and it's in the public qRound overloads. Then, add a private qSaturateRound which re-uses the casting, clamps to range ("saturating" -- which is still casting to the closest integer value), and casts to the final type (only int, at the moment). This new API is then used in the geometry classes when they round. Fixes: QTBUG-133261 Change-Id: Iba0d9baa17c136a976a945742c117e4cfac520b3 Reviewed-by: Thiago Macieira --- src/corelib/global/qnumeric.h | 73 +++++++++++++------ src/corelib/tools/qmargins.h | 23 ++++-- src/corelib/tools/qpoint.h | 20 ++--- src/corelib/tools/qrect.h | 8 +- src/corelib/tools/qsize.h | 14 ++-- .../corelib/tools/qpointf/tst_qpointf.cpp | 5 ++ 6 files changed, 91 insertions(+), 52 deletions(-) diff --git a/src/corelib/global/qnumeric.h b/src/corelib/global/qnumeric.h index 31e0fa69164..70a03db4350 100644 --- a/src/corelib/global/qnumeric.h +++ b/src/corelib/global/qnumeric.h @@ -487,41 +487,66 @@ constexpr inline auto qUnsignedAbs(T t) using U = std::make_unsigned_t; return (t >= 0) ? U(t) : U(~U(t) + U(1)); } -} // namespace QtPrivate +namespace QRoundImpl { // gcc < 10 doesn't have __has_builtin #if defined(Q_PROCESSOR_ARM_64) && (__has_builtin(__builtin_round) || defined(Q_CC_GNU)) && !defined(Q_CC_CLANG) // ARM64 has a single instruction that can do C++ rounding with conversion to integer. // Note current clang versions have non-constexpr __builtin_round, ### allow clang this path when they fix it. -constexpr inline int qRound(double d) -{ return int(__builtin_round(d)); } -constexpr inline int qRound(float f) -{ return int(__builtin_roundf(f)); } -constexpr inline qint64 qRound64(double d) -{ return qint64(__builtin_round(d)); } -constexpr inline qint64 qRound64(float f) -{ return qint64(__builtin_roundf(f)); } +constexpr inline double qRound(double d) +{ return __builtin_round(d); } +constexpr inline float qRound(float f) +{ return __builtin_roundf(f); } #elif defined(__SSE2__) && (__has_builtin(__builtin_copysign) || defined(Q_CC_GNU)) // SSE has binary operations directly on floating point making copysign fast -constexpr inline int qRound(double d) -{ return int(d + __builtin_copysign(0.5, d)); } -constexpr inline int qRound(float f) -{ return int(f + __builtin_copysignf(0.5f, f)); } -constexpr inline qint64 qRound64(double d) -{ return qint64(d + __builtin_copysign(0.5, d)); } -constexpr inline qint64 qRound64(float f) -{ return qint64(f + __builtin_copysignf(0.5f, f)); } +constexpr inline double qRound(double d) +{ return d + __builtin_copysign(0.5, d); } +constexpr inline float qRound(float f) +{ return f + __builtin_copysignf(0.5f, f); } #else +constexpr inline double qRound(double d) +{ return d >= 0.0 ? d + 0.5 : d - 0.5; } +constexpr inline float qRound(float d) +{ return d >= 0.0f ? d + 0.5f : d - 0.5f; } +#endif +} // namespace QRoundImpl + +// Like qRound, but have well-defined saturating behavior. +// NaN is not handled. +template , bool> = true> +constexpr inline int qSaturateRound(FP value) +{ +#ifdef QT_SUPPORTS_IS_CONSTANT_EVALUATED + if (!q20::is_constant_evaluated()) + Q_ASSERT(!qIsNaN(value)); +#endif + constexpr FP MinBound = FP((std::numeric_limits::min)()); + constexpr FP MaxBound = FP((std::numeric_limits::max)()); + const FP beforeTruncation = QRoundImpl::qRound(value); + return int(qBound(MinBound, beforeTruncation, MaxBound)); +} +} // namespace QtPrivate + constexpr inline int qRound(double d) -{ return d >= 0.0 ? int(d + 0.5) : int(d - 0.5); } -constexpr inline int qRound(float d) -{ return d >= 0.0f ? int(d + 0.5f) : int(d - 0.5f); } +{ + return int(QtPrivate::QRoundImpl::qRound(d)); +} + +constexpr inline int qRound(float f) +{ + return int(QtPrivate::QRoundImpl::qRound(f)); +} constexpr inline qint64 qRound64(double d) -{ return d >= 0.0 ? qint64(d + 0.5) : qint64(d - 0.5); } -constexpr inline qint64 qRound64(float d) -{ return d >= 0.0f ? qint64(d + 0.5f) : qint64(d - 0.5f); } -#endif +{ + return qint64(QtPrivate::QRoundImpl::qRound(d)); +} + +constexpr inline qint64 qRound64(float f) +{ + return qint64(QtPrivate::QRoundImpl::qRound(f)); +} namespace QtPrivate { template diff --git a/src/corelib/tools/qmargins.h b/src/corelib/tools/qmargins.h index fb3d70f64fe..f833a338b16 100644 --- a/src/corelib/tools/qmargins.h +++ b/src/corelib/tools/qmargins.h @@ -198,14 +198,18 @@ constexpr inline QMargins operator*(int factor, const QMargins &margins) noexcep constexpr inline QMargins operator*(const QMargins &margins, qreal factor) noexcept { // Deliberately using left(), top() etc. (checked ints don't have FP arithmetic) - return QMargins(qRound(margins.left() * factor), qRound(margins.top() * factor), - qRound(margins.right() * factor), qRound(margins.bottom() * factor)); + return QMargins(QtPrivate::qSaturateRound(margins.left() * factor), + QtPrivate::qSaturateRound(margins.top() * factor), + QtPrivate::qSaturateRound(margins.right() * factor), + QtPrivate::qSaturateRound(margins.bottom() * factor)); } constexpr inline QMargins operator*(qreal factor, const QMargins &margins) noexcept { - return QMargins(qRound(margins.left() * factor), qRound(margins.top() * factor), - qRound(margins.right() * factor), qRound(margins.bottom() * factor)); + return QMargins(QtPrivate::qSaturateRound(margins.left() * factor), + QtPrivate::qSaturateRound(margins.top() * factor), + QtPrivate::qSaturateRound(margins.right() * factor), + QtPrivate::qSaturateRound(margins.bottom() * factor)); } constexpr inline QMargins operator/(const QMargins &margins, int divisor) @@ -217,8 +221,10 @@ constexpr inline QMargins operator/(const QMargins &margins, int divisor) constexpr inline QMargins operator/(const QMargins &margins, qreal divisor) { Q_ASSERT(!qFuzzyIsNull(divisor)); - return QMargins(qRound(margins.left() / divisor), qRound(margins.top() / divisor), - qRound(margins.right() / divisor), qRound(margins.bottom() / divisor)); + return QMargins(QtPrivate::qSaturateRound(margins.left() / divisor), + QtPrivate::qSaturateRound(margins.top() / divisor), + QtPrivate::qSaturateRound(margins.right() / divisor), + QtPrivate::qSaturateRound(margins.bottom() / divisor)); } constexpr inline QMargins operator|(const QMargins &m1, const QMargins &m2) noexcept @@ -533,7 +539,10 @@ constexpr QMarginsF QMargins::toMarginsF() const noexcept { return *this; } constexpr inline QMargins QMarginsF::toMargins() const noexcept { - return QMargins(qRound(m_left), qRound(m_top), qRound(m_right), qRound(m_bottom)); + return QMargins(QtPrivate::qSaturateRound(m_left), + QtPrivate::qSaturateRound(m_top), + QtPrivate::qSaturateRound(m_right), + QtPrivate::qSaturateRound(m_bottom)); } #ifndef QT_NO_DEBUG_STREAM diff --git a/src/corelib/tools/qpoint.h b/src/corelib/tools/qpoint.h index c652ef1daf8..6465cbdfd67 100644 --- a/src/corelib/tools/qpoint.h +++ b/src/corelib/tools/qpoint.h @@ -65,15 +65,15 @@ private: friend constexpr inline QPoint operator-(const QPoint &p1, const QPoint &p2) noexcept { return QPoint(p1.xp - p2.xp, p1.yp - p2.yp); } friend constexpr inline QPoint operator*(const QPoint &p, float factor) - { return QPoint(qRound(p.x() * factor), qRound(p.y() * factor)); } + { return QPoint(QtPrivate::qSaturateRound(p.x() * factor), QtPrivate::qSaturateRound(p.y() * factor)); } friend constexpr inline QPoint operator*(const QPoint &p, double factor) - { return QPoint(qRound(p.x() * factor), qRound(p.y() * factor)); } + { return QPoint(QtPrivate::qSaturateRound(p.x() * factor), QtPrivate::qSaturateRound(p.y() * factor)); } friend constexpr inline QPoint operator*(const QPoint &p, int factor) noexcept { return QPoint(p.xp * factor, p.yp * factor); } friend constexpr inline QPoint operator*(float factor, const QPoint &p) - { return QPoint(qRound(p.x() * factor), qRound(p.y() * factor)); } + { return QPoint(QtPrivate::qSaturateRound(p.x() * factor), QtPrivate::qSaturateRound(p.y() * factor)); } friend constexpr inline QPoint operator*(double factor, const QPoint &p) - { return QPoint(qRound(p.x() * factor), qRound(p.y() * factor)); } + { return QPoint(QtPrivate::qSaturateRound(p.x() * factor), QtPrivate::qSaturateRound(p.y() * factor)); } friend constexpr inline QPoint operator*(int factor, const QPoint &p) noexcept { return QPoint(p.xp * factor, p.yp * factor); } friend constexpr inline QPoint operator+(const QPoint &p) noexcept @@ -83,7 +83,7 @@ private: friend constexpr inline QPoint operator/(const QPoint &p, qreal c) { Q_ASSERT(!qFuzzyIsNull(c)); - return QPoint(qRound(p.x() / c), qRound(p.y() / c)); + return QPoint(QtPrivate::qSaturateRound(p.x() / c), QtPrivate::qSaturateRound(p.y() / c)); } public: @@ -189,15 +189,15 @@ constexpr inline QPoint &QPoint::operator-=(const QPoint &p) constexpr inline QPoint &QPoint::operator*=(float factor) { - xp.setValue(qRound(x() * factor)); - yp.setValue(qRound(y() * factor)); + xp.setValue(QtPrivate::qSaturateRound(x() * factor)); + yp.setValue(QtPrivate::qSaturateRound(y() * factor)); return *this; } constexpr inline QPoint &QPoint::operator*=(double factor) { - xp.setValue(qRound(x() * factor)); - yp.setValue(qRound(y() * factor)); + xp.setValue(QtPrivate::qSaturateRound(x() * factor)); + yp.setValue(QtPrivate::qSaturateRound(y() * factor)); return *this; } @@ -415,7 +415,7 @@ constexpr QPointF QPoint::toPointF() const noexcept { return *this; } constexpr inline QPoint QPointF::toPoint() const { - return QPoint(qRound(xp), qRound(yp)); + return QPoint(QtPrivate::qSaturateRound(xp), QtPrivate::qSaturateRound(yp)); } #ifndef QT_NO_DEBUG_STREAM diff --git a/src/corelib/tools/qrect.h b/src/corelib/tools/qrect.h index 04f3b9a6d67..ddab318bf06 100644 --- a/src/corelib/tools/qrect.h +++ b/src/corelib/tools/qrect.h @@ -889,10 +889,10 @@ constexpr inline QRect QRectF::toRect() const noexcept // This rounding is designed to minimize the maximum possible difference // in topLeft(), bottomRight(), and size() after rounding. // All dimensions are at most off by 0.75, and topLeft by at most 0.5. - const int nxp = qRound(xp); - const int nyp = qRound(yp); - const int nw = qRound(w + (xp - nxp) / 2); - const int nh = qRound(h + (yp - nyp) / 2); + const int nxp = QtPrivate::qSaturateRound(xp); + const int nyp = QtPrivate::qSaturateRound(yp); + const int nw = QtPrivate::qSaturateRound(w + (xp - nxp) / 2); + const int nh = QtPrivate::qSaturateRound(h + (yp - nyp) / 2); return QRect(nxp, nyp, nw, nh); } diff --git a/src/corelib/tools/qsize.h b/src/corelib/tools/qsize.h index 75f1f8e70fb..86509cb6483 100644 --- a/src/corelib/tools/qsize.h +++ b/src/corelib/tools/qsize.h @@ -69,13 +69,13 @@ private: friend inline constexpr QSize operator-(const QSize &s1, const QSize &s2) noexcept { return QSize(s1.wd - s2.wd, s1.ht - s2.ht); } friend inline constexpr QSize operator*(const QSize &s, qreal c) noexcept - { return QSize(qRound(s.width() * c), qRound(s.height() * c)); } + { return QSize(QtPrivate::qSaturateRound(s.width() * c), QtPrivate::qSaturateRound(s.height() * c)); } friend inline constexpr QSize operator*(qreal c, const QSize &s) noexcept { return s * c; } friend inline QSize operator/(const QSize &s, qreal c) { Q_ASSERT(!qFuzzyIsNull(c)); - return QSize(qRound(s.width() / c), qRound(s.height() / c)); + return QSize(QtPrivate::qSaturateRound(s.width() / c), QtPrivate::qSaturateRound(s.height() / c)); } friend inline constexpr size_t qHash(const QSize &, size_t) noexcept; @@ -183,8 +183,8 @@ constexpr inline QSize &QSize::operator-=(const QSize &s) noexcept constexpr inline QSize &QSize::operator*=(qreal c) noexcept { - wd.setValue(qRound(width() * c)); - ht.setValue(qRound(height() * c)); + wd.setValue(QtPrivate::qSaturateRound(width() * c)); + ht.setValue(QtPrivate::qSaturateRound(height() * c)); return *this; } @@ -194,8 +194,8 @@ constexpr inline size_t qHash(const QSize &s, size_t seed = 0) noexcept inline QSize &QSize::operator/=(qreal c) { Q_ASSERT(!qFuzzyIsNull(c)); - wd.setValue(qRound(width() / c)); - ht.setValue(qRound(height() / c)); + wd.setValue(QtPrivate::qSaturateRound(width() / c)); + ht.setValue(QtPrivate::qSaturateRound(height() / c)); return *this; } @@ -410,7 +410,7 @@ constexpr inline QSizeF QSizeF::boundedTo(const QSizeF &otherSize) const noexcep constexpr inline QSize QSizeF::toSize() const noexcept { - return QSize(qRound(wd), qRound(ht)); + return QSize(QtPrivate::qSaturateRound(wd), QtPrivate::qSaturateRound(ht)); } constexpr QSizeF QSize::toSizeF() const noexcept { return *this; } diff --git a/tests/auto/corelib/tools/qpointf/tst_qpointf.cpp b/tests/auto/corelib/tools/qpointf/tst_qpointf.cpp index b4cb6c5d289..c4241e90e7f 100644 --- a/tests/auto/corelib/tools/qpointf/tst_qpointf.cpp +++ b/tests/auto/corelib/tools/qpointf/tst_qpointf.cpp @@ -29,6 +29,9 @@ CHECK(const &&); #include +#include +#include + class tst_QPointF : public QObject { Q_OBJECT @@ -414,6 +417,8 @@ void tst_QPointF::toPoint_data() QTest::newRow("(0.0, 0.0) ==> (0, 0)") << QPointF(0, 0) << QPoint(0, 0); QTest::newRow("(0.5, 0.5) ==> (1, 1)") << QPointF(0.5, 0.5) << QPoint(1, 1); QTest::newRow("(-0.5, -0.5) ==> (-1, -1)") << QPointF(-0.5, -0.5) << QPoint(-1, -1); + QTest::newRow("(DBL_MAX, -DBL_MAX) ==> (INT_MAX, INT_MIN)") << QPointF(DBL_MAX, -DBL_MAX) << QPoint(INT_MAX, INT_MIN); + QTest::newRow("(HUGE_VAL, 0) ==> (INT_MAX, 0)") << QPointF(HUGE_VAL, 0) << QPoint(INT_MAX, 0); } void tst_QPointF::toPoint()