From 49113c905d5868e6b76bb6b7b3e0a20b0c56a23a Mon Sep 17 00:00:00 2001 From: Volker Hilsheimer Date: Mon, 1 Mar 2021 11:44:28 +0100 Subject: Don't access moved-from object Clang's static analyzer tooling warns about this access-after-move [1]. While the comment above the function indicated that this was deliberate and relying on a moved-from QString being valid, it is still bad practice. Since 'str' is empty in moved-from state if - and only if - it was a non-const reference, make a compile-time check of the constness of type T instead. [1] https://testresults.qt.io/codechecker/daily_analyses/qtbase/dev/qtbase-dev-20210301-e14ccf0553/qstring.cpp_clang-tidy_b0545db57a2cc5dac67a56f76322ffd0.plist.html#reportHash=209ee3db0b17d21919326a1ad6635318 Pick-to: 6.1 Change-Id: Iac1813b61b6a3c2ef4053b911a4043c5382f85e4 Reviewed-by: Thiago Macieira Reviewed-by: Edward Welbourne Reviewed-by: Andrei Golubev --- src/corelib/text/qstring.cpp | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/corelib/text/qstring.cpp b/src/corelib/text/qstring.cpp index a6f3cd2b5d..390c10e1fc 100644 --- a/src/corelib/text/qstring.cpp +++ b/src/corelib/text/qstring.cpp @@ -6342,9 +6342,9 @@ namespace QUnicodeTables { \endlist In copy-convert mode, the local variable \c{s} is detached from the input - \a str. In the in-place convert mode, \a str is in moved-from state (which - this function requires to be a valid, empty string) and \c{s} contains the - only copy of the string, without reallocation (thus, \a it is still valid). + \a str. In the in-place convert mode, \a str is in moved-from state and + \c{s} contains the only copy of the string, without reallocation (thus, + \a it is still valid). There is one pathological case left: when the in-place conversion needs to reallocate memory to grow the buffer. In that case, we need to adjust the \a @@ -6355,7 +6355,7 @@ Q_NEVER_INLINE static QString detachAndConvertCase(T &str, QStringIterator it, QUnicodeTables::Case which) { Q_ASSERT(!str.isEmpty()); - QString s = std::move(str); // will copy if T is const QString + QString s = std::move(str); // will copy if T is const QString QChar *pp = s.begin() + it.index(); // will detach if necessary do { @@ -6374,9 +6374,8 @@ static QString detachAndConvertCase(T &str, QStringIterator it, QUnicodeTables:: s.replace(outpos, 1, reinterpret_cast(folded.data()), folded.size()); pp = const_cast(s.constBegin()) + outpos + folded.size(); - // do we need to adjust the input iterator too? - // if it is pointing to s's data, str is empty - if (str.isEmpty()) + // Adjust the input iterator if we are performing an in-place conversion + if constexpr (!std::is_const::value) it = QStringIterator(s.constBegin(), inpos + folded.size(), s.constEnd()); } } else { -- cgit v1.2.3 From e718818745e6db8df09ea55c4071e116f00851c9 Mon Sep 17 00:00:00 2001 From: Volker Hilsheimer Date: Thu, 25 Feb 2021 14:19:34 +0100 Subject: QLabel: simplify createStandardContextMenu If control is nullptr in the beginning, then it will be nullptr later as well, and the function won't do anything. And if the effectiveTextFormat is Qt::PlainText, then the linkToCopy will be empty, and the function won't do anything, either. So we can just handle these cases right away, making the code simplier. Fixes static analyzer warning 43de3f3125108b4353afd94e94f59926. Pick-to: 6.1 Change-Id: I5b8eb94a1e40c2725de6a168298d8f3bcde748eb Reviewed-by: David Skoland Reviewed-by: Richard Moe Gustavsen --- src/widgets/widgets/qlabel.cpp | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/widgets/widgets/qlabel.cpp b/src/widgets/widgets/qlabel.cpp index 2d7b7f79b1..189a7488d3 100644 --- a/src/widgets/widgets/qlabel.cpp +++ b/src/widgets/widgets/qlabel.cpp @@ -1694,14 +1694,13 @@ QPoint QLabelPrivate::layoutPoint(const QPoint& p) const #ifndef QT_NO_CONTEXTMENU QMenu *QLabelPrivate::createStandardContextMenu(const QPoint &pos) { - QString linkToCopy; - QPoint p; - if (control && effectiveTextFormat != Qt::PlainText) { - p = layoutPoint(pos); - linkToCopy = control->document()->documentLayout()->anchorAt(p); - } + if (!control || effectiveTextFormat == Qt::PlainText) + return nullptr; + + const QPoint p = layoutPoint(pos); + QString linkToCopy = control->document()->documentLayout()->anchorAt(p); - if (linkToCopy.isEmpty() && !control) + if (linkToCopy.isEmpty()) return nullptr; return control->createStandardContextMenu(p, q_func()); -- cgit v1.2.3