From b8b675014f4f38cd3cee15584edd3b561bc0846e Mon Sep 17 00:00:00 2001 From: Ahmad Samir Date: Tue, 25 Oct 2022 09:19:51 +0200 Subject: [PATCH] QString: don't detach in remove(QChar ch, Qt::CaseSensitivity cs) - If the string isn't shared, don't call detach(), instead remove characters matching ch, and resize() - If the string is shared, create a new string, and copy all characters except the ones that would be removed, see task for details Update unittets so that calls to this overload of remove() test both code paths (replace() calls remove(QChar, cs) internally). Drive-by change: use QCOMPARE() instead of QTEST() Task-number: QTBUG-106181 Change-Id: I1fa08cf29baac2560fca62861fc4a81967b54e92 Reviewed-by: Thiago Macieira --- src/corelib/text/qstring.cpp | 21 ++++++++--- .../auto/corelib/text/qstring/tst_qstring.cpp | 36 ++++++++++++++----- 2 files changed, 44 insertions(+), 13 deletions(-) diff --git a/src/corelib/text/qstring.cpp b/src/corelib/text/qstring.cpp index cbaa9a4c36f..3dcb812a806 100644 --- a/src/corelib/text/qstring.cpp +++ b/src/corelib/text/qstring.cpp @@ -3416,13 +3416,26 @@ QString &QString::remove(QChar ch, Qt::CaseSensitivity cs) return ch == (isCase ? x : x.toCaseFolded()); }; - detach(); + auto begin = d.begin(); auto first_match = begin + idx; auto end = d.end(); - auto it = std::remove_if(first_match, end, match); - d->erase(it, std::distance(it, end)); - d.data()[d.size] = u'\0'; + if (!d->isShared()) { + auto it = std::remove_if(first_match, end, match); + d->erase(it, std::distance(it, end)); + d.data()[d.size] = u'\0'; + } else { + // Instead of detaching, create a new string and copy all characters except for + // the ones we're removing + // TODO: size() is more than the needed since "copy" would be shorter + QString copy{size(), Qt::Uninitialized}; + auto dst = copy.d.begin(); + auto it = std::copy(begin, first_match, dst); // Chunk before idx + it = std::remove_copy_if(first_match + 1, end, it, match); + copy.d.size = std::distance(dst, it); + copy.d.data()[copy.d.size] = u'\0'; + *this = copy; + } return *this; } diff --git a/tests/auto/corelib/text/qstring/tst_qstring.cpp b/tests/auto/corelib/text/qstring/tst_qstring.cpp index b164c846cd7..230182889c1 100644 --- a/tests/auto/corelib/text/qstring/tst_qstring.cpp +++ b/tests/auto/corelib/text/qstring/tst_qstring.cpp @@ -830,6 +830,7 @@ void tst_QString::replace_string_data() QTest::newRow( "rem20" ) << QString("a") << QString("A") << QString("") << QString("") << false; QTest::newRow( "rem21" ) << QString("A") << QString("a") << QString("") << QString("") << false; QTest::newRow( "rem22" ) << QString("Alpha beta") << QString("a") << QString("") << QString("lph bet") << false; + QTest::newRow( "rem23" ) << QString("+00:00") << QString(":") << QString("") << QString("+0000") << false; QTest::newRow( "rep00" ) << QString("ABC") << QString("B") << QString("-") << QString("A-C") << true; QTest::newRow( "rep01" ) << QString("$()*+.?[\\]^{|}") << QString("$()*+.?[\\]^{|}") << QString("X") << QString("X") << true; @@ -3208,26 +3209,35 @@ void tst_QString::replace_string() QFETCH( QString, before ); QFETCH( QString, after ); QFETCH( bool, bcs ); + QFETCH(QString, result); Qt::CaseSensitivity cs = bcs ? Qt::CaseSensitive : Qt::CaseInsensitive; if ( before.size() == 1 ) { QChar ch = before.at( 0 ); + // Test when isShared() is true QString s1 = string; s1.replace( ch, after, cs ); - QTEST( s1, "result" ); + QCOMPARE(s1, result); + QString s4 = string; + s4.begin(); // Test when isShared() is false + s4.replace(ch, after, cs); + QCOMPARE(s4, result); + + // What is this one testing? it calls the same replace() overload + // as the previous two if ( QChar(ch.toLatin1()) == ch ) { QString s2 = string; s2.replace( ch.toLatin1(), after, cs ); - QTEST( s2, "result" ); + QCOMPARE(s2, result); } } QString s3 = string; s3.replace( before, after, cs ); - QTEST( s3, "result" ); + QCOMPARE(s3, result); } void tst_QString::replace_string_extra() @@ -3343,6 +3353,7 @@ void tst_QString::remove_string() QFETCH( QString, before ); QFETCH( QString, after ); QFETCH( bool, bcs ); + QFETCH(QString, result); Qt::CaseSensitivity cs = bcs ? Qt::CaseSensitive : Qt::CaseInsensitive; @@ -3350,28 +3361,35 @@ void tst_QString::remove_string() if ( before.size() == 1 && cs ) { QChar ch = before.at( 0 ); + // Test when isShared() is true QString s1 = string; s1.remove( ch ); - QTEST( s1, "result" ); + QCOMPARE(s1, result); + // Test again with isShared() is false + QString s4 = string; + s4.begin(); // Detach + s4.remove( ch ); + QCOMPARE(s4, result); + + // What is this one testing? it calls the same remove() overload + // as the previous two if ( QChar(ch.toLatin1()) == ch ) { QString s2 = string; s2.remove( ch ); - QTEST( s2, "result" ); + QCOMPARE(s2, result); } } QString s3 = string; s3.remove( before, cs ); - QTEST( s3, "result" ); + QCOMPARE(s3, result); if (QtPrivate::isLatin1(before)) { QString s6 = string; s6.remove( QLatin1String(before.toLatin1()), cs ); - QTEST( s6, "result" ); + QCOMPARE(s6, result); } - } else { - QCOMPARE( 0, 0 ); // shut Qt Test } }