QString/QByteArray: detach immediately in operator[]

Unlike any other implicitly shared container, QString/QByteArray
have a "lazy detach" mechanism: their operator[] returns a
special object; assignment into that object will actually
detach.

In other words:

  QString a("Hello");
  QCharRef c = a[0];  // does not detach
  c = 'J';            // detach happens here

This allows this behavior:

  QString a("Hello");
  QCharRef c = a[0];
  QString b = a;
  c = 'J';               // detach happens here
  assert(a == "Jello");
  assert(b == "Hello");

Note that this happens only with operator[] -- the mutating
iterator APIs instead detach immediately, making the above code
have visible side effects in b (at the end, b == "Jello").

The reasons for this special behavior seems to have been lost in
the dawn of time: this is something present all the way back
since Qt 2, maybe even Qt 1. Holding on to a "reference" while
taking copies of a container is documented [1] to be a bad idea,
so we shouldn't double check that the users don't do it.

This patch:

1) adds an immediate detach in operator[], just like all other
containers;

2) adds a warning in debug builds in case QByteRef/QCharRef is going
to cause a detach;

3) marks operator[] as [[nodiscard]] to warn users not using
Clazy about the (unintended) detach now happening in their code.

This paves the way for removal of QCharRef/QByteRef, likely in
Qt 7.

[1] https://doc.qt.io/qt-5/containers.html#implicit-sharing-iterator-problem

[ChangeLog][QtCore][QString] QString::operator[] detaches
immediately. Previously, the detach was delayed until a
modification was made to the string through the returned
QCharRef.

[ChangeLog][QtCore][QByteArray] QByteArray::operator[] detaches
immediately. Previously, the detach was delayed until a
modification was made to the byte array through the returned
QByteRef.

Change-Id: I9f77ae36759d80dc3202426a798f5b1e5fb2c2c5
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>
 
 
 
 
 
 
 

4 files changed, 52 insertions, 24 deletions

diff --git a/src/corelib/tools/qbytearray.cpp b/src/corelib/tools/qbytearray.cpp
index c89fb078f7..9816b5cb32 100644
--- a/src/corelib/tools/qbytearray.cpp
+++ b/src/corelib/tools/qbytearray.cpp
@@ -1540,8 +1540,11 @@ QByteArray &QByteArray::operator=(const char *str)
     \note Before Qt 5.14 it was possible to use this operator to access
     a character at an out-of-bounds position in the byte array, and
     then assign to such position, causing the byte array to be
-    automatically resized. This behavior is deprecated, and will be
-    changed in a future version of Qt.
+    automatically resized. Furthermore, assigning a value to the
+    returned QByteRef would cause a detach of the byte array, even if the
+    byte array has been copied in the meanwhile (and the QByteRef kept
+    alive while the copy was taken). These behaviors are deprecated,
+    and will be changed in a future version of Qt.
 
     \sa at()
 */
@@ -5062,10 +5065,15 @@ QByteArray QByteArray::toPercentEncoding(const QByteArray &exclude, const QByteA
 
 namespace QtPrivate {
 namespace DeprecatedRefClassBehavior {
-void warn(EmittingClass c)
+void warn(WarningType w, EmittingClass c)
 {
+    static const char deprecatedBehaviorString[] =
+            "The corresponding behavior is deprecated, and will be changed"
+             " in a future version of Qt.";
+
     const char *emittingClassName = nullptr;
     const char *containerClassName = nullptr;
+
     switch (c) {
     case EmittingClass::QByteRef:
         emittingClassName = "QByteRef";
@@ -5077,12 +5085,16 @@ void warn(EmittingClass c)
         break;
     }
 
-    qWarning("Using %s with an index pointing outside"
-             " the valid range of a %s."
-             " The corresponding behavior is deprecated, and will be changed"
-             " in a future version of Qt.",
-             emittingClassName,
-             containerClassName);
+    switch (w) {
+    case WarningType::OutOfRange:
+        qWarning("Using %s with an index pointing outside the valid range of a %s. %s",
+                 emittingClassName, containerClassName, deprecatedBehaviorString);
+        break;
+    case WarningType::DelayedDetach:
+        qWarning("Using %s with on a %s that is not already detached. %s",
+                 emittingClassName, containerClassName, deprecatedBehaviorString);
+        break;
+    }
 }
 } // namespace DeprecatedRefClassBehavior
 } // namespace QtPrivate
diff --git a/src/corelib/tools/qbytearray.h b/src/corelib/tools/qbytearray.h
index 5c7229e40d..a81051d309 100644
--- a/src/corelib/tools/qbytearray.h
+++ b/src/corelib/tools/qbytearray.h
@@ -209,8 +209,8 @@ public:
     inline char at(int i) const;
     inline char operator[](int i) const;
     inline char operator[](uint i) const;
-    inline QByteRef operator[](int i);
-    inline QByteRef operator[](uint i);
+    Q_REQUIRED_RESULT inline QByteRef operator[](int i);
+    Q_REQUIRED_RESULT inline QByteRef operator[](uint i);
     Q_REQUIRED_RESULT char front() const { return at(0); }
     Q_REQUIRED_RESULT inline QByteRef front();
     Q_REQUIRED_RESULT char back() const { return at(size() - 1); }
@@ -535,7 +535,12 @@ namespace DeprecatedRefClassBehavior {
         QCharRef,
     };
 
-    Q_CORE_EXPORT Q_DECL_COLD_FUNCTION void warn(EmittingClass c);
+    enum class WarningType {
+        OutOfRange,
+        DelayedDetach,
+    };
+
+    Q_CORE_EXPORT Q_DECL_COLD_FUNCTION void warn(WarningType w, EmittingClass c);
 } // namespace DeprecatedAssignmentOperatorBehavior
 } // namespace QtPrivate
 
@@ -556,7 +561,7 @@ public:
         if (Q_LIKELY(i < a.d->size))
             return a.d->data()[i];
 #ifdef QT_DEBUG
-        warn(EmittingClass::QByteRef);
+        warn(WarningType::OutOfRange, EmittingClass::QByteRef);
 #endif
         return char(0);
     }
@@ -565,10 +570,14 @@ public:
         using namespace QtPrivate::DeprecatedRefClassBehavior;
         if (Q_UNLIKELY(i >= a.d->size)) {
 #ifdef QT_DEBUG
-            warn(EmittingClass::QByteRef);
+            warn(WarningType::OutOfRange, EmittingClass::QByteRef);
 #endif
             a.expand(i);
         } else {
+#ifdef QT_DEBUG
+            if (Q_UNLIKELY(!a.isDetached()))
+                warn(WarningType::DelayedDetach, EmittingClass::QByteRef);
+#endif
             a.detach();
         }
         a.d->data()[i] = c;
@@ -593,9 +602,9 @@ public:
 };
 
 inline QByteRef QByteArray::operator[](int i)
-{ Q_ASSERT(i >= 0); return QByteRef(*this, i); }
+{ Q_ASSERT(i >= 0); detach(); return QByteRef(*this, i); }
 inline QByteRef QByteArray::operator[](uint i)
-{ return QByteRef(*this, i); }
+{  detach(); return QByteRef(*this, i); }
 inline QByteRef QByteArray::front() { return operator[](0); }
 inline QByteRef QByteArray::back() { return operator[](size() - 1); }
 inline QByteArray::iterator QByteArray::begin()
diff --git a/src/corelib/tools/qstring.cpp b/src/corelib/tools/qstring.cpp
index 9b28d17876..aa602559fe 100644
--- a/src/corelib/tools/qstring.cpp
+++ b/src/corelib/tools/qstring.cpp
@@ -5785,8 +5785,11 @@ QString QString::trimmed_helper(QString &str)
     \note Before Qt 5.14 it was possible to use this operator to access
     a character at an out-of-bounds position in the string, and
     then assign to such position, causing the string to be
-    automatically resized. This behavior is deprecated, and will be
-    changed in a future version of Qt.
+    automatically resized. Furthermore, assigning a value to the
+    returned QCharRef would cause a detach of the string, even if the
+    string has been copied in the meanwhile (and the QCharRef kept
+    alive while the copy was taken). These behaviors are deprecated,
+    and will be changed in a future version of Qt.
 
     \sa at()
 */
diff --git a/src/corelib/tools/qstring.h b/src/corelib/tools/qstring.h
index 5bc3a87832..0f7b015bef 100644
--- a/src/corelib/tools/qstring.h
+++ b/src/corelib/tools/qstring.h
@@ -271,9 +271,9 @@ public:
 
     inline const QChar at(int i) const;
     const QChar operator[](int i) const;
-    QCharRef operator[](int i);
+    Q_REQUIRED_RESULT QCharRef operator[](int i);
     const QChar operator[](uint i) const;
-    QCharRef operator[](uint i);
+    Q_REQUIRED_RESULT QCharRef operator[](uint i);
 
     Q_REQUIRED_RESULT inline QChar front() const { return at(0); }
     Q_REQUIRED_RESULT inline QCharRef front();
@@ -1089,7 +1089,7 @@ public:
         if (Q_LIKELY(i < s.d->size))
             return s.d->data()[i];
 #ifdef QT_DEBUG
-        warn(EmittingClass::QCharRef);
+        warn(WarningType::OutOfRange, EmittingClass::QCharRef);
 #endif
         return 0;
     }
@@ -1098,10 +1098,14 @@ public:
         using namespace QtPrivate::DeprecatedRefClassBehavior;
         if (Q_UNLIKELY(i >= s.d->size)) {
 #ifdef QT_DEBUG
-            warn(EmittingClass::QCharRef);
+            warn(WarningType::OutOfRange, EmittingClass::QCharRef);
 #endif
             s.resize(i + 1, QLatin1Char(' '));
         } else {
+#ifdef QT_DEBUG
+            if (Q_UNLIKELY(!s.isDetached()))
+                warn(WarningType::DelayedDetach, EmittingClass::QCharRef);
+#endif
             s.detach();
         }
         s.d->data()[i] = c.unicode();
@@ -1215,9 +1219,9 @@ inline void QString::squeeze()
 inline QString &QString::setUtf16(const ushort *autf16, int asize)
 { return setUnicode(reinterpret_cast<const QChar *>(autf16), asize); }
 inline QCharRef QString::operator[](int i)
-{ Q_ASSERT(i >= 0); return QCharRef(*this, i); }
+{ Q_ASSERT(i >= 0); detach(); return QCharRef(*this, i); }
 inline QCharRef QString::operator[](uint i)
-{ return QCharRef(*this, i); }
+{  detach(); return QCharRef(*this, i); }
 inline QCharRef QString::front() { return operator[](0); }
 inline QCharRef QString::back() { return operator[](size() - 1); }
 inline QString::iterator QString::begin()