Deal with {und,ov}erflow issues in QLine's length handling

Use std::hypot() instead of sqrt() of a sum of squares.
This ensures length() can't be zero when isNull() is false.
Use length() in QLine::setLength() rather than duplicating that.

Clarify and expand some documentation; isNull() never said what
constituted validity, nor did unitVector() mention that is should not
be used on a line for which isNull() is true. Make clear that lines of
denormal length cannot be rescaled accurately.

Given that we use fuzzy comparison to determine equality of
end-points, isNull() can be false for a line with displacements less
than sqrt(numeric_limits<qreal>::denorm_min()) between the coordinates
of its end-points (as long as these are not much bigger); squaring
these would give zero, hence a zero length, where using hypot() avoids
the underflow and gives a non-zero length. Having a zero length for a
line with isNull() false would lead to problems in setLength(), which
uses an isNull() pre-test, protecting a call to unitVector().

(It was already possible for a null line to have non-zero length; this
now arises in more cases.)

Reworked QLine::setLength() to allow for the possibility that the unit
vector it computes as transient may not have length exactly one.
Add tests against {ov,und}erflow and divide-by-zero problems in QLine.

Picked version for 5.12 needed to adapt the test to simulate 5.15 and
later's QCOMPARE(act, exp) effectively testing qFuzzyIsNull(exp) ?
qFuzzyIsNull(act) : qFuzzyCompare(act, exp) where 5.12 just uses plain
qFuzzyCompare().

Change-Id: I7b71d66b872ccc08a64e941acd36b45b0ea15fab
Reviewed-by: Sze Howe Koh <szehowe.koh@gmail.com>
(cherry picked from commit 1c591fd9246ca776304a3c370dd2578bd886feac)
Reviewed-by: Paul Wicking <paul.wicking@qt.io>
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>

3 files changed, 73 insertions, 31 deletions

diff --git a/src/corelib/tools/qline.cpp b/src/corelib/tools/qline.cpp
index 949f63ea15..24801a7404 100644
--- a/src/corelib/tools/qline.cpp
+++ b/src/corelib/tools/qline.cpp
@@ -1,6 +1,6 @@
 /****************************************************************************
 **
-** Copyright (C) 2016 The Qt Company Ltd.
+** Copyright (C) 2020 The Qt Company Ltd.
 ** Contact: https://www.qt.io/licensing/
 **
 ** This file is part of the QtCore module of the Qt Toolkit.
@@ -38,6 +38,7 @@
 ****************************************************************************/
 
 #include "qline.h"
+
 #include "qdebug.h"
 #include "qdatastream.h"
 #include "qmath.h"
@@ -99,7 +100,7 @@ QT_BEGIN_NAMESPACE
 /*!
     \fn bool QLine::isNull() const
 
-    Returns \c true if the line is not set up with valid start and end point;
+    Returns \c true if the line does not have distinct start and end points;
     otherwise returns \c false.
 */
 
@@ -428,8 +429,14 @@ QDataStream &operator>>(QDataStream &stream, QLine &line)
 /*!
     \fn bool QLineF::isNull() const
 
-    Returns \c true if the line is not set up with valid start and end point;
-    otherwise returns \c false.
+    Returns \c true if the line does not have distinct start and end points;
+    otherwise returns \c false. The start and end points are considered distinct
+    if qFuzzyCompare() can distinguish them in at least one coordinate.
+
+    \note Due to the use of fuzzy comparison, isNull() may return \c true for
+    lines whose length() is not zero.
+
+    \sa qFuzzyCompare(), length()
 */
 
 /*!
@@ -512,10 +519,10 @@ QDataStream &operator>>(QDataStream &stream, QLine &line)
     Sets the length of the line to the given \a length. QLineF will
     move the end point - p2() - of the line to give the line its new length.
 
-    If the line is a null line, the length will remain zero regardless
-    of the length specified.
+    A null line will not be rescaled. For non-null lines with very short lengths
+    (represented by denormal floating-point values), results may be imprecise.
 
-    \sa length(), isNull()
+    \sa length(), isNull(), unitVector()
 */
 
 /*!
@@ -560,13 +567,12 @@ QDataStream &operator>>(QDataStream &stream, QLine &line)
 /*!
     Returns the length of the line.
 
-    \sa setLength()
+    \sa setLength(), isNull()
 */
 qreal QLineF::length() const
 {
-    qreal x = pt2.x() - pt1.x();
-    qreal y = pt2.y() - pt1.y();
-    return qSqrt(x*x + y*y);
+    using std::hypot;
+    return hypot(dx(), dy());
 }
 
 /*!
@@ -637,16 +643,18 @@ QLineF QLineF::fromPolar(qreal length, qreal angle)
 
 /*!
     Returns the unit vector for this line, i.e a line starting at the
-    same point as \e this line with a length of 1.0.
+    same point as \e this line with a length of 1.0, provided the line
+    is non-null.
 
-    \sa normalVector()
+    \sa normalVector(), setLength()
 */
 QLineF QLineF::unitVector() const
 {
-    qreal x = pt2.x() - pt1.x();
-    qreal y = pt2.y() - pt1.y();
+    qreal x = dx();
+    qreal y = dy();
+    using std::hypot;
+    qreal len = hypot(x, y);
 
-    qreal len = qSqrt(x*x + y*y);
     QLineF f(p1(), QPointF(pt1.x() + x/len, pt1.y() + y/len));
 
 #ifndef QT_NO_DEBUG
diff --git a/src/corelib/tools/qline.h b/src/corelib/tools/qline.h
index 6361c1af9f..f15ee40856 100644
--- a/src/corelib/tools/qline.h
+++ b/src/corelib/tools/qline.h
@@ -1,6 +1,6 @@
 /****************************************************************************
 **
-** Copyright (C) 2016 The Qt Company Ltd.
+** Copyright (C) 2020 The Qt Company Ltd.
 ** Contact: https://www.qt.io/licensing/
 **
 ** This file is part of the QtCore module of the Qt Toolkit.
@@ -375,7 +375,9 @@ inline void QLineF::setLength(qreal len)
 {
     if (isNull())
         return;
-    QLineF v = unitVector();
+    Q_ASSERT(length() > 0);
+    const QLineF v = unitVector();
+    len /= v.length(); // In case it's not quite exactly 1.
     pt2 = QPointF(pt1.x() + v.dx() * len, pt1.y() + v.dy() * len);
 }
 
diff --git a/tests/auto/corelib/tools/qline/tst_qline.cpp b/tests/auto/corelib/tools/qline/tst_qline.cpp
index ae65d8f697..21dc8b9434 100644
--- a/tests/auto/corelib/tools/qline/tst_qline.cpp
+++ b/tests/auto/corelib/tools/qline/tst_qline.cpp
@@ -1,6 +1,6 @@
 /****************************************************************************
 **
-** Copyright (C) 2016 The Qt Company Ltd.
+** Copyright (C) 2020 The Qt Company Ltd.
 ** Contact: https://www.qt.io/licensing/
 **
 ** This file is part of the test suite of the Qt Toolkit.
@@ -225,10 +225,12 @@ void tst_QLine::testLength_data()
     QTest::addColumn<double>("vx");
     QTest::addColumn<double>("vy");
 
-    QTest::newRow("[1,0]*2") << 0.0 << 0.0 << 1.0 << 0.0 << 1.0 << 2.0 << 2.0 << 0.0;
-    QTest::newRow("[0,1]*2") << 0.0 << 0.0 << 0.0 << 1.0 << 1.0 << 2.0 << 0.0 << 2.0;
-    QTest::newRow("[-1,0]*2") << 0.0 << 0.0 << -1.0 << 0.0 << 1.0 << 2.0 << -2.0 << 0.0;
-    QTest::newRow("[0,-1]*2") << 0.0 << 0.0 << 0.0 << -1.0 << 1.0 << 2.0 << 0.0 << -2.0;
+    // Test name: [dx,dy]->|lenToSet| (x1,x2)
+    // with the last part omitted if (0,0)
+    QTest::newRow("[1,0]->|2|") << 0.0 << 0.0 << 1.0 << 0.0 << 1.0 << 2.0 << 2.0 << 0.0;
+    QTest::newRow("[0,1]->|2|") << 0.0 << 0.0 << 0.0 << 1.0 << 1.0 << 2.0 << 0.0 << 2.0;
+    QTest::newRow("[-1,0]->|2|") << 0.0 << 0.0 << -1.0 << 0.0 << 1.0 << 2.0 << -2.0 << 0.0;
+    QTest::newRow("[0,-1]->|2|") << 0.0 << 0.0 << 0.0 << -1.0 << 1.0 << 2.0 << 0.0 << -2.0;
     QTest::newRow("[1,1]->|1|") << 0.0 << 0.0 << 1.0 << 1.0
                              << double(SQRT2) << 1.0 << double(UNITX_45) << double(UNITX_45);
     QTest::newRow("[-1,1]->|1|") << 0.0 << 0.0 << -1.0 << 1.0
@@ -237,10 +239,10 @@ void tst_QLine::testLength_data()
                              << double(SQRT2) << 1.0 << double(UNITX_45) << double(-UNITX_45);
     QTest::newRow("[-1,-1]->|1|") << 0.0 << 0.0 << -1.0 << -1.0
                              << double(SQRT2) << 1.0 << double(-UNITX_45) << double(-UNITX_45);
-    QTest::newRow("[1,0]*2 (2,2)") << 2.0 << 2.0 << 3.0 << 2.0 << 1.0 << 2.0 << 2.0 << 0.0;
-    QTest::newRow("[0,1]*2 (2,2)") << 2.0 << 2.0 << 2.0 << 3.0 << 1.0 << 2.0 << 0.0 << 2.0;
-    QTest::newRow("[-1,0]*2 (2,2)") << 2.0 << 2.0 << 1.0 << 2.0 << 1.0 << 2.0 << -2.0 << 0.0;
-    QTest::newRow("[0,-1]*2 (2,2)") << 2.0 << 2.0 << 2.0 << 1.0 << 1.0 << 2.0 << 0.0 << -2.0;
+    QTest::newRow("[1,0]->|2| (2,2)") << 2.0 << 2.0 << 3.0 << 2.0 << 1.0 << 2.0 << 2.0 << 0.0;
+    QTest::newRow("[0,1]->|2| (2,2)") << 2.0 << 2.0 << 2.0 << 3.0 << 1.0 << 2.0 << 0.0 << 2.0;
+    QTest::newRow("[-1,0]->|2| (2,2)") << 2.0 << 2.0 << 1.0 << 2.0 << 1.0 << 2.0 << -2.0 << 0.0;
+    QTest::newRow("[0,-1]->|2| (2,2)") << 2.0 << 2.0 << 2.0 << 1.0 << 1.0 << 2.0 << 0.0 << -2.0;
     QTest::newRow("[1,1]->|1| (2,2)") << 2.0 << 2.0 << 3.0 << 3.0
                                    << double(SQRT2) << 1.0 << double(UNITX_45) << double(UNITX_45);
     QTest::newRow("[-1,1]->|1| (2,2)") << 2.0 << 2.0 << 1.0 << 3.0
@@ -249,6 +251,20 @@ void tst_QLine::testLength_data()
                                     << double(SQRT2) << 1.0 << double(UNITX_45) << double(-UNITX_45);
     QTest::newRow("[-1,-1]->|1| (2,2)") << 2.0 << 2.0 << 1.0 << 1.0
                                      << double(SQRT2) << 1.0 << double(-UNITX_45) << double(-UNITX_45);
+    const double small = qSqrt(std::numeric_limits<qreal>::denorm_min()) / 8;
+    QTest::newRow("[small,small]->|2| (-small/2,-small/2)")
+        << -(small * .5) << -(small * .5) << (small * .5) << (small * .5)
+        << (small * M_SQRT2) << (2 * M_SQRT2) << 2.0 << 2.0;
+    const double tiny = std::numeric_limits<qreal>::min() / 2;
+    QTest::newRow("[tiny,tiny]->|2| (-tiny/2,-tiny/2)")
+        << -(tiny * .5) << -(tiny * .5) << (tiny * .5) << (tiny * .5)
+        << (tiny * M_SQRT2) << (2 * M_SQRT2) << 2.0 << 2.0;
+    QTest::newRow("[1+3e-13,1+4e-13]|1895| (1, 1)")
+        << 1.0 << 1.0 << (1 + 3e-13) << (1 + 4e-13) // isNull(), so ignores setLength()
+        << 5e-13 << 1895.0 << 3e-13 << 4e-13;
+    QTest::newRow("[4e-323,5e-324]|1892|") // Unavoidable underflow: denormals
+        << 0.0 << 0.0 << 4e-323 << 5e-324
+        << 4e-323 << 1892.0 << 4e-323 << 5e-324; // vx, vy values ignored
 }
 
 void tst_QLine::testLength()
@@ -263,12 +279,28 @@ void tst_QLine::testLength()
     QFETCH(double, vy);
 
     QLineF l(x1, y1, x2, y2);
-    QCOMPARE(l.length(), qreal(length));
+    const bool wasNull = l.isNull();
+    if (!wasNull)
+        QCOMPARE(l.length(), qreal(length));
 
     l.setLength(lengthToSet);
-    QCOMPARE(l.length(), qreal(lengthToSet));
-    QCOMPARE(l.dx(), qreal(vx));
-    QCOMPARE(l.dy(), qreal(vy));
+    if (!wasNull)
+        QCOMPARE(l.length(), qreal(lengthToSet));
+    else if (qFuzzyIsNull(qreal(length)))
+        QVERIFY(qFuzzyIsNull(l.length()));
+    else
+        QCOMPARE(l.length(), qreal(length));
+    // Scaling tiny values up to big can be imprecise: don't try to test vx, vy
+    if (wasNull || !qFuzzyIsNull(length)) {
+        if (qFuzzyIsNull(qreal(vx)))
+            QVERIFY(qFuzzyIsNull(l.dx()));
+        else
+            QCOMPARE(l.dx(), qreal(vx));
+        if (qFuzzyIsNull(qreal(vy)))
+            QVERIFY(qFuzzyIsNull(l.dy()));
+        else
+            QCOMPARE(l.dy(), qreal(vy));
+    }
 }
 
 void tst_QLine::testCenter()