QString: fix lifetime issues with QRegularExpression APIs
QString has several functions taking a QRegularExpression: indexOf(), contains(), and so on. Some of those have an out-argument of type QRegularExpressionMatch, to report the details of the match (if any). For instance: QRegularExpression re(...); QRegularExpressionMatch match; if (string.contains(re, &match)) use(match); The code used to route the implementation of these functions through QStringView (which has the very same functions). This however opens up a lifetime problem with temporary strings: if (getString().contains(re, &match)) use(match); // match is dangling Here `match` is dangling because it is referencing data into the destroyed temporary -- nothing is keeping the string alive. This is against the rules we've decided for Qt, and it's also asymmetric with the corresponding code that uses QRegularExpression directly instead: match = re.match(getString()); if (match.hasMatch()) use(match); // not dangling ... although we've documented not to do this. (In light of the decision we've made w.r.t. temporaries, the documentation is wrong anyways.) Here QRE takes a copy of the string and stores it in the match object, thus keeping it alive. Hence, extend the implementation of the QString functions to keep a (shallow) copy of the string. To keep the code shared as much as possible with QStringView, in theory one could have a function taking a std::variant<QString, QStringView> and that uses the currently active member. However I've found that std::variant here creates some abysmal codegen, so instead I went for a simpler approach -- pass a QStringView and an optional pointer to a QString. Use the latter if it's loaded. QStringView has some inline code that calls into exported functions, so I can't change the signature of them without breaking BC; I'm instead adding new unexported functions and a Qt 7 note to unify them. Change-Id: I7c65885a84069d0fbb902dcc96ddff543ca84562 Fixes: QTBUG-103940 Reviewed-by: Thiago Macieira <thiago.macieira@intel.com> (cherry picked from commit 6f852935e0c847f91c7d6ffca455a74307ebe26d) Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>
This commit is contained in:
parent
25695a4826
commit
c5685f6654
@ -4560,7 +4560,7 @@ qsizetype QString::count(QStringView str, Qt::CaseSensitivity cs) const
|
|||||||
*/
|
*/
|
||||||
qsizetype QString::indexOf(const QRegularExpression &re, qsizetype from, QRegularExpressionMatch *rmatch) const
|
qsizetype QString::indexOf(const QRegularExpression &re, qsizetype from, QRegularExpressionMatch *rmatch) const
|
||||||
{
|
{
|
||||||
return QtPrivate::indexOf(QStringView(*this), re, from, rmatch);
|
return QtPrivate::indexOf(QStringView(*this), this, re, from, rmatch);
|
||||||
}
|
}
|
||||||
|
|
||||||
/*!
|
/*!
|
||||||
@ -4594,7 +4594,7 @@ qsizetype QString::indexOf(const QRegularExpression &re, qsizetype from, QRegula
|
|||||||
*/
|
*/
|
||||||
qsizetype QString::lastIndexOf(const QRegularExpression &re, qsizetype from, QRegularExpressionMatch *rmatch) const
|
qsizetype QString::lastIndexOf(const QRegularExpression &re, qsizetype from, QRegularExpressionMatch *rmatch) const
|
||||||
{
|
{
|
||||||
return QtPrivate::lastIndexOf(QStringView(*this), re, from, rmatch);
|
return QtPrivate::lastIndexOf(QStringView(*this), this, re, from, rmatch);
|
||||||
}
|
}
|
||||||
|
|
||||||
/*!
|
/*!
|
||||||
@ -4633,7 +4633,7 @@ qsizetype QString::lastIndexOf(const QRegularExpression &re, qsizetype from, QRe
|
|||||||
|
|
||||||
bool QString::contains(const QRegularExpression &re, QRegularExpressionMatch *rmatch) const
|
bool QString::contains(const QRegularExpression &re, QRegularExpressionMatch *rmatch) const
|
||||||
{
|
{
|
||||||
return QtPrivate::contains(QStringView(*this), re, rmatch);
|
return QtPrivate::contains(QStringView(*this), this, re, rmatch);
|
||||||
}
|
}
|
||||||
|
|
||||||
/*!
|
/*!
|
||||||
@ -10798,14 +10798,16 @@ qsizetype QtPrivate::lastIndexOf(QLatin1StringView haystack, qsizetype from, QLa
|
|||||||
}
|
}
|
||||||
|
|
||||||
#if QT_CONFIG(regularexpression)
|
#if QT_CONFIG(regularexpression)
|
||||||
qsizetype QtPrivate::indexOf(QStringView haystack, const QRegularExpression &re, qsizetype from, QRegularExpressionMatch *rmatch)
|
qsizetype QtPrivate::indexOf(QStringView viewHaystack, const QString *stringHaystack, const QRegularExpression &re, qsizetype from, QRegularExpressionMatch *rmatch)
|
||||||
{
|
{
|
||||||
if (!re.isValid()) {
|
if (!re.isValid()) {
|
||||||
qtWarnAboutInvalidRegularExpression(re.pattern(), "QString(View)::indexOf");
|
qtWarnAboutInvalidRegularExpression(re.pattern(), "QString(View)::indexOf");
|
||||||
return -1;
|
return -1;
|
||||||
}
|
}
|
||||||
|
|
||||||
QRegularExpressionMatch match = re.match(haystack, from);
|
QRegularExpressionMatch match = stringHaystack
|
||||||
|
? re.match(*stringHaystack, from)
|
||||||
|
: re.match(viewHaystack, from);
|
||||||
if (match.hasMatch()) {
|
if (match.hasMatch()) {
|
||||||
const qsizetype ret = match.capturedStart();
|
const qsizetype ret = match.capturedStart();
|
||||||
if (rmatch)
|
if (rmatch)
|
||||||
@ -10816,15 +10818,22 @@ qsizetype QtPrivate::indexOf(QStringView haystack, const QRegularExpression &re,
|
|||||||
return -1;
|
return -1;
|
||||||
}
|
}
|
||||||
|
|
||||||
qsizetype QtPrivate::lastIndexOf(QStringView haystack, const QRegularExpression &re, qsizetype from, QRegularExpressionMatch *rmatch)
|
qsizetype QtPrivate::indexOf(QStringView haystack, const QRegularExpression &re, qsizetype from, QRegularExpressionMatch *rmatch)
|
||||||
|
{
|
||||||
|
return indexOf(haystack, nullptr, re, from, rmatch);
|
||||||
|
}
|
||||||
|
|
||||||
|
qsizetype QtPrivate::lastIndexOf(QStringView viewHaystack, const QString *stringHaystack, const QRegularExpression &re, qsizetype from, QRegularExpressionMatch *rmatch)
|
||||||
{
|
{
|
||||||
if (!re.isValid()) {
|
if (!re.isValid()) {
|
||||||
qtWarnAboutInvalidRegularExpression(re.pattern(), "QString(View)::lastIndexOf");
|
qtWarnAboutInvalidRegularExpression(re.pattern(), "QString(View)::lastIndexOf");
|
||||||
return -1;
|
return -1;
|
||||||
}
|
}
|
||||||
|
|
||||||
qsizetype endpos = (from < 0) ? (haystack.size() + from + 1) : (from + 1);
|
qsizetype endpos = (from < 0) ? (viewHaystack.size() + from + 1) : (from + 1);
|
||||||
QRegularExpressionMatchIterator iterator = re.globalMatch(haystack);
|
QRegularExpressionMatchIterator iterator = stringHaystack
|
||||||
|
? re.globalMatch(*stringHaystack)
|
||||||
|
: re.globalMatch(viewHaystack);
|
||||||
qsizetype lastIndex = -1;
|
qsizetype lastIndex = -1;
|
||||||
while (iterator.hasNext()) {
|
while (iterator.hasNext()) {
|
||||||
QRegularExpressionMatch match = iterator.next();
|
QRegularExpressionMatch match = iterator.next();
|
||||||
@ -10841,19 +10850,31 @@ qsizetype QtPrivate::lastIndexOf(QStringView haystack, const QRegularExpression
|
|||||||
return lastIndex;
|
return lastIndex;
|
||||||
}
|
}
|
||||||
|
|
||||||
bool QtPrivate::contains(QStringView haystack, const QRegularExpression &re, QRegularExpressionMatch *rmatch)
|
qsizetype QtPrivate::lastIndexOf(QStringView haystack, const QRegularExpression &re, qsizetype from, QRegularExpressionMatch *rmatch)
|
||||||
|
{
|
||||||
|
return lastIndexOf(haystack, nullptr, re, from, rmatch);
|
||||||
|
}
|
||||||
|
|
||||||
|
bool QtPrivate::contains(QStringView viewHaystack, const QString *stringHaystack, const QRegularExpression &re, QRegularExpressionMatch *rmatch)
|
||||||
{
|
{
|
||||||
if (!re.isValid()) {
|
if (!re.isValid()) {
|
||||||
qtWarnAboutInvalidRegularExpression(re.pattern(), "QString(View)::contains");
|
qtWarnAboutInvalidRegularExpression(re.pattern(), "QString(View)::contains");
|
||||||
return false;
|
return false;
|
||||||
}
|
}
|
||||||
QRegularExpressionMatch m = re.match(haystack);
|
QRegularExpressionMatch m = stringHaystack
|
||||||
|
? re.match(*stringHaystack)
|
||||||
|
: re.match(viewHaystack);
|
||||||
bool hasMatch = m.hasMatch();
|
bool hasMatch = m.hasMatch();
|
||||||
if (hasMatch && rmatch)
|
if (hasMatch && rmatch)
|
||||||
*rmatch = std::move(m);
|
*rmatch = std::move(m);
|
||||||
return hasMatch;
|
return hasMatch;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
bool QtPrivate::contains(QStringView haystack, const QRegularExpression &re, QRegularExpressionMatch *rmatch)
|
||||||
|
{
|
||||||
|
return contains(haystack, nullptr, re, rmatch);
|
||||||
|
}
|
||||||
|
|
||||||
qsizetype QtPrivate::count(QStringView haystack, const QRegularExpression &re)
|
qsizetype QtPrivate::count(QStringView haystack, const QRegularExpression &re)
|
||||||
{
|
{
|
||||||
if (!re.isValid()) {
|
if (!re.isValid()) {
|
||||||
|
@ -69,14 +69,30 @@ namespace QtPrivate {
|
|||||||
[[nodiscard]] Q_CORE_EXPORT Q_DECL_PURE_FUNCTION qsizetype count(QLatin1StringView haystack, QChar needle, Qt::CaseSensitivity cs = Qt::CaseSensitive) noexcept;
|
[[nodiscard]] Q_CORE_EXPORT Q_DECL_PURE_FUNCTION qsizetype count(QLatin1StringView haystack, QChar needle, Qt::CaseSensitivity cs = Qt::CaseSensitive) noexcept;
|
||||||
|
|
||||||
#if QT_CONFIG(regularexpression)
|
#if QT_CONFIG(regularexpression)
|
||||||
|
// ### Qt 7: unify these overloads;
|
||||||
|
// remove the ones taking only a QStringView, export the others, adjust callers
|
||||||
|
[[nodiscard]] qsizetype indexOf(QStringView viewHaystack,
|
||||||
|
const QString *stringHaystack,
|
||||||
|
const QRegularExpression &re,
|
||||||
|
qsizetype from = 0,
|
||||||
|
QRegularExpressionMatch *rmatch = nullptr);
|
||||||
[[nodiscard]] Q_CORE_EXPORT qsizetype indexOf(QStringView haystack,
|
[[nodiscard]] Q_CORE_EXPORT qsizetype indexOf(QStringView haystack,
|
||||||
const QRegularExpression &re,
|
const QRegularExpression &re,
|
||||||
qsizetype from = 0,
|
qsizetype from = 0,
|
||||||
QRegularExpressionMatch *rmatch = nullptr);
|
QRegularExpressionMatch *rmatch = nullptr);
|
||||||
|
[[nodiscard]] qsizetype lastIndexOf(QStringView viewHaystack,
|
||||||
|
const QString *stringHaystack,
|
||||||
|
const QRegularExpression &re,
|
||||||
|
qsizetype from = -1,
|
||||||
|
QRegularExpressionMatch *rmatch = nullptr);
|
||||||
[[nodiscard]] Q_CORE_EXPORT qsizetype lastIndexOf(QStringView haystack,
|
[[nodiscard]] Q_CORE_EXPORT qsizetype lastIndexOf(QStringView haystack,
|
||||||
const QRegularExpression &re,
|
const QRegularExpression &re,
|
||||||
qsizetype from = -1,
|
qsizetype from = -1,
|
||||||
QRegularExpressionMatch *rmatch = nullptr);
|
QRegularExpressionMatch *rmatch = nullptr);
|
||||||
|
[[nodiscard]] bool contains(QStringView viewHaystack,
|
||||||
|
const QString *stringHaystack,
|
||||||
|
const QRegularExpression &re,
|
||||||
|
QRegularExpressionMatch *rmatch = nullptr);
|
||||||
[[nodiscard]] Q_CORE_EXPORT bool contains(QStringView haystack,
|
[[nodiscard]] Q_CORE_EXPORT bool contains(QStringView haystack,
|
||||||
const QRegularExpression &re,
|
const QRegularExpression &re,
|
||||||
QRegularExpressionMatch *rmatch = nullptr);
|
QRegularExpressionMatch *rmatch = nullptr);
|
||||||
|
@ -537,6 +537,7 @@ private slots:
|
|||||||
#if QT_CONFIG(regularexpression)
|
#if QT_CONFIG(regularexpression)
|
||||||
void split_regularexpression_data();
|
void split_regularexpression_data();
|
||||||
void split_regularexpression();
|
void split_regularexpression();
|
||||||
|
void regularexpression_lifetime();
|
||||||
#endif
|
#endif
|
||||||
void fromUtf16_data();
|
void fromUtf16_data();
|
||||||
void fromUtf16();
|
void fromUtf16();
|
||||||
@ -6244,6 +6245,45 @@ void tst_QString::split_regularexpression()
|
|||||||
split_regexp<QStringList, QRegularExpression>(string, pattern, result);
|
split_regexp<QStringList, QRegularExpression>(string, pattern, result);
|
||||||
split_regexp<QList<QStringView>, QRegularExpression>(string, pattern, result);
|
split_regexp<QList<QStringView>, QRegularExpression>(string, pattern, result);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Test that rvalue strings (e.g. temporaries) are kept alive in
|
||||||
|
// QRegularExpression-related APIs
|
||||||
|
void tst_QString::regularexpression_lifetime()
|
||||||
|
{
|
||||||
|
const auto getString = [] {
|
||||||
|
// deliberately heap-allocated
|
||||||
|
return QString(QLatin1String("the quick brown fox jumps over the lazy dog"));
|
||||||
|
};
|
||||||
|
|
||||||
|
QRegularExpression re("\\w{5}");
|
||||||
|
|
||||||
|
{
|
||||||
|
QString s = getString();
|
||||||
|
QRegularExpressionMatch match;
|
||||||
|
const bool contains = std::move(s).contains(re, &match);
|
||||||
|
s.fill('X'); // NOLINT(bugprone-use-after-move)
|
||||||
|
QVERIFY(contains);
|
||||||
|
QCOMPARE(match.capturedView(), u"quick");
|
||||||
|
}
|
||||||
|
|
||||||
|
{
|
||||||
|
QString s = getString();
|
||||||
|
QRegularExpressionMatch match;
|
||||||
|
const auto index = std::move(s).indexOf(re, 0, &match);
|
||||||
|
s.fill('X'); // NOLINT(bugprone-use-after-move)
|
||||||
|
QCOMPARE(index, 4);
|
||||||
|
QCOMPARE(match.capturedView(), u"quick");
|
||||||
|
}
|
||||||
|
|
||||||
|
{
|
||||||
|
QString s = getString();
|
||||||
|
QRegularExpressionMatch match;
|
||||||
|
const auto lastIndex = std::move(s).lastIndexOf(re, &match);
|
||||||
|
s.fill('X'); // NOLINT(bugprone-use-after-move)
|
||||||
|
QCOMPARE(lastIndex, 20);
|
||||||
|
QCOMPARE(match.capturedView(), u"jumps");
|
||||||
|
}
|
||||||
|
}
|
||||||
#endif
|
#endif
|
||||||
|
|
||||||
void tst_QString::fromUtf16_data()
|
void tst_QString::fromUtf16_data()
|
||||||
|
Loading…
x
Reference in New Issue
Block a user