1berry/qtbase
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Geometry classes: honor the documentation when it talks about rounding

Browse Source 
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 <thiago.macieira@intel.com>
This commit is contained in:
Giuseppe D'Angelo 2025-02-05 18:33:25 +01:00
parent 177cd123d2
commit c5608c675a
6 changed files with 91 additions and 52 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

73
src/corelib/global/qnumeric.h
View File

@ -487,41 +487,66 @@ constexpr inline auto qUnsignedAbs(T t)
using U = std::make_unsigned_t<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 <typename FP,
typename std::enable_if_t<std::is_floating_point_v<FP>, 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<int>::min)());
constexpr FP MaxBound = FP((std::numeric_limits<int>::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 <typename T>

23
src/corelib/tools/qmargins.h
View File

@ -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

20
src/corelib/tools/qpoint.h
View File

@ -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

8
src/corelib/tools/qrect.h
View File

@ -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);
}

14
src/corelib/tools/qsize.h
View File

@ -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; }

5
tests/auto/corelib/tools/qpointf/tst_qpointf.cpp
View File

@ -29,6 +29,9 @@ CHECK(const &&);
#include <qpoint.h>
#include <cmath>
#include <cfloat>
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()