Skip to content

Replace QRegExp with QRegularExpression#606

Closed
jarolrod wants to merge 1 commit into
bitcoin-core:masterfrom
jarolrod:regexp-obsolete
Closed

Replace QRegExp with QRegularExpression#606
jarolrod wants to merge 1 commit into
bitcoin-core:masterfrom
jarolrod:regexp-obsolete

Conversation

@jarolrod

@jarolrod jarolrod commented May 24, 2022

Copy link
Copy Markdown
Contributor

Picking up #585

Replaces occurrences of QRegExp usage with QRegularExpression as part of the roadmap for Qt6 integration.

fixes #578

@jarolrod

Copy link
Copy Markdown
Contributor Author

ping: @hebasto @promag

@hebasto hebasto left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK.

Suggesting to apply clang-format-diff.py and use QStringLiteral macro:

--- a/src/qt/utilitydialog.cpp
+++ b/src/qt/utilitydialog.cpp
@@ -23,6 +23,7 @@
 #include <QLabel>
 #include <QMainWindow>
 #include <QRegularExpression>
+#include <QString>
 #include <QTextCursor>
 #include <QTextTable>
 #include <QVBoxLayout>
@@ -44,8 +45,8 @@ HelpMessageDialog::HelpMessageDialog(QWidget *parent, bool about) :
         /// HTML-format the license message from the core
         QString licenseInfoHTML = QString::fromStdString(LicenseInfo());
         // Make URLs clickable
-        QRegularExpression uri("<(.*)>", QRegularExpression::InvertedGreedinessOption);
-        licenseInfoHTML.replace(uri, "<a href=\"\\1\">\\1</a>");
+        QRegularExpression uri(QStringLiteral("<(.*)>"), QRegularExpression::InvertedGreedinessOption);
+        licenseInfoHTML.replace(uri, QStringLiteral("<a href=\"\\1\">\\1</a>"));
         // Replace newlines with HTML breaks
         licenseInfoHTML.replace("\n", "<br>");
 

Still reviewing guiutil.cpp changes.

@hebasto

hebasto commented May 25, 2022

Copy link
Copy Markdown
Member

Echoing: #585 (comment)

@hebasto

hebasto commented May 25, 2022

Copy link
Copy Markdown
Member

It seems the code is broken both in master and in this branch. It does not do what is supposed to do

/* Extract first suffix from filter pattern "Description (*.foo)" or "Description (*.foo *.bar ...) */

I mean, it fails to extract first suffix from filter pattern "Description (*.foo *.bar ...)".

However, all of the callers in our codebase do not use patterns with multiple suffixes.

@hebasto

hebasto commented May 25, 2022

Copy link
Copy Markdown
Member

I mean, it fails to extract first suffix from filter pattern "Description (*.foo *.bar ...)".

Ok, an inversion of greediness will help:

--- a/src/qt/guiutil.cpp
+++ b/src/qt/guiutil.cpp
@@ -311,7 +311,7 @@ QString getSaveFileName(QWidget *parent, const QString &caption, const QString &
     QString result = QDir::toNativeSeparators(QFileDialog::getSaveFileName(parent, caption, myDir, filter, &selectedFilter));
 
     /* Extract first suffix from filter pattern "Description (*.foo)" or "Description (*.foo *.bar ...) */
-    QRegularExpression filter_re(".* \\(\\*\\.(.*)[ \\)]");
+    QRegularExpression filter_re(QStringLiteral(".* \\(\\*\\.(.*)[ \\)]"), QRegularExpression::InvertedGreedinessOption);
     QString selectedSuffix;
     QRegularExpressionMatch m = filter_re.match(selectedFilter);
     if(m.hasMatch())

@promag

promag commented May 26, 2022

Copy link
Copy Markdown
Contributor

Echoing: #585 (comment)

Agree.

@DrahtBot

DrahtBot commented Jun 12, 2022

Copy link
Copy Markdown
Contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

@hebasto

hebasto commented Jun 20, 2022

Copy link
Copy Markdown
Member

Labeled "Up for grabs"...

@hebasto

hebasto commented Jun 20, 2022

Copy link
Copy Markdown
Member

Closing in favor of #620.

@hebasto hebasto closed this Jun 20, 2022
laanwj added a commit to bitcoin/bitcoin that referenced this pull request Jun 22, 2022
67364eb test, qt: Add tests for `GUIUtil::extractFirstSuffixFromFilter` (w0xlt)
ace9af5 qt: Replace `QRegExp` with `QRegularExpression` (w0xlt)
c378535 qt: Add a function that extracts the suffix from a filter (w0xlt)

Pull request description:

  Picking up bitcoin-core/gui#606 (labeled "Up for grabs") and applying bitcoin-core/gui#606 (review) and bitcoin-core/gui#606 (comment).

  Replaces occurrences of `QRegExp` usage with `QRegularExpression` as part of the roadmap for Qt6 integration.

  Fixes bitcoin-core/gui#578

ACKs for top commit:
  laanwj:
    Code review and lightly tested ACK 67364eb
  hebasto:
    ACK 67364eb

Tree-SHA512: 4a17d83e557bc635cbd1a15776856e9edb7162b23a369ccbd2ac59c68b8a1ea663baaa7d5ad98e419dc03b91ef3315c768eeadc01c0b29162de109493161e814
@bitcoin-core bitcoin-core locked and limited conversation to collaborators Jun 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Replace QRegExp with QRegularExpression

6 participants