Replace `QRegExp` with `QRegularExpression` #620

pull w0xlt wants to merge 3 commits into bitcoin-core:master from w0xlt:regexp-obsolete changing 5 files +38 −19

w0xlt commented at 1:19 PM on June 20, 2022: contributor

Picking up #606 (labeled "Up for grabs") and applying #606#pullrequestreview-984607067 and #606 (comment).

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

Fixes https://github.com/bitcoin-core/gui/issues/578
hebasto commented at 2:47 PM on June 20, 2022: member

@w0xlt

Thank you for picking it up!

What is your opinion about #606 (comment)?
hebasto renamed this:
~~qt: replace `QRegExp` with `QRegularExpression`~~
Replace `QRegExp` with `QRegularExpression`
on Jun 20, 2022
hebasto added the label Refactoring on Jun 20, 2022
hebasto added the label Qt 6 on Jun 20, 2022
w0xlt commented at 3:36 AM on June 21, 2022: contributor

What is your opinion about #606 (comment)?

That looks good to me. I'll take a look at it. Would you suggest a class/file to allocate the utility function?
hebasto commented at 7:11 AM on June 21, 2022: member

What is your opinion about #606 (comment)?

That looks good to me. I'll take a look at it. Would you suggest a class/file to allocate the utility function?

namespace GUIUtil in qt/guiutil.{h,cpp}?
w0xlt force-pushed on Jun 21, 2022
w0xlt force-pushed on Jun 21, 2022
w0xlt commented at 3:40 PM on June 21, 2022: contributor

Force-pushed https://github.com/bitcoin-core/gui/pull/620/commits/c0c8b33748637372ee5b267f6bbcce33a5380f4d addressing #606 (comment).
in src/qt/test/optiontests.cpp:139 in c0c8b33748 outdated
```
 134 | +
 135 | +    filter = QString("PNG Image (*.png)");
 136 | +    QCOMPARE(GUIUtil::extractFirstSuffixFromFilter(filter), "png");
 137 | +
 138 | +    filter = QString("Wallet Data (*.dat)");
 139 | +    QCOMPARE(GUIUtil::extractFirstSuffixFromFilter(filter), "dat");
```
hebasto commented at 7:43 PM on June 21, 2022:

Why 4 tests with the only suffix? Maybe just two tests? First test with one suffix, another one with two suffixes.

w0xlt commented at 10:30 PM on June 21, 2022:

Done in https://github.com/bitcoin-core/gui/pull/620/commits/67364ebe4c499eb8effe8dac11a5e3648f30c6c7.

in src/qt/test/optiontests.cpp:131 in c0c8b33748 outdated

 135 | +    filter = QString("PNG Image (*.png)");
 136 | +    QCOMPARE(GUIUtil::extractFirstSuffixFromFilter(filter), "png");
 137 | +
 138 | +    filter = QString("Wallet Data (*.dat)");
 139 | +    QCOMPARE(GUIUtil::extractFirstSuffixFromFilter(filter), "dat");
 140 | +

hebasto commented at 7:43 PM on June 21, 2022:

Drop empty line?

w0xlt commented at 10:30 PM on June 21, 2022:

Done in https://github.com/bitcoin-core/gui/commit/67364ebe4c499eb8effe8dac11a5e3648f30c6c7.

in src/qt/utilitydialog.cpp:25 in c0c8b33748 outdated
```
  21 | @@ -22,7 +22,7 @@
  22 |  #include <QCloseEvent>
  23 |  #include <QLabel>
  24 |  #include <QMainWindow>
  25 | -#include <QRegExp>
  26 | +#include <QRegularExpression>
```
hebasto commented at 7:44 PM on June 21, 2022:

Add #include <QString> as well (due to using QStringLiteral)?

w0xlt commented at 10:30 PM on June 21, 2022:

Done in https://github.com/bitcoin-core/gui/pull/620/commits/ace9af5688662a56d855bd26f36bc5c145b909e3.
hebasto commented at 7:46 PM on June 21, 2022: member
Approach ACK c0c8b33748637372ee5b267f6bbcce33a5380f4d.

Suggesting to split the first commit into separated ones:
- factoring out the extractFirstSuffixFromFilter() function (I'd prefer to use UpperCamelCase name convention for this new standalone function)
- replacement QRegExp with QRegularExpression
Also please apply clang-format-diff.py for each your commit.

qt: Add a function that extracts the suffix from a filter

Extract the 'Extract first suffix from filter pattern...'
functionality into a testable utility function

c378535e28

qt: Replace `QRegExp` with `QRegularExpression`

Co-authored-by: Pavol Rusnak <pavol@rusnak.io>
Co-authored-by: Jarol Rodriguez <jarolrod@tutanota.com>

ace9af5688

test, qt: Add tests for `GUIUtil::extractFirstSuffixFromFilter` 67364ebe4c

in src/qt/guiutil.h:130 in c0c8b33748 outdated

 122 | @@ -123,6 +123,13 @@ namespace GUIUtil
 123 |       */
 124 |      QString getDefaultDataDirectory();
 125 |  
 126 | +
 127 | +    /** Extract first suffix from filter pattern "Description (*.foo)" or "Description (*.foo *.bar ...).
 128 | +
 129 | +      @param[in] filter Filter specification such as "Comma Separated Files (*.csv)"
 130 | +    */

hebasto commented at 7:49 PM on June 21, 2022:

Please consult with Coding Style (Doxygen-compatible comments).

w0xlt commented at 10:31 PM on June 21, 2022:

Done in https://github.com/bitcoin-core/gui/pull/620/commits/c378535e28e0c54353fde9c4a66cb3134a3ee742.

w0xlt force-pushed on Jun 21, 2022
w0xlt commented at 10:32 PM on June 21, 2022: contributor

@hebasto thanks for the review. Force-pushed https://github.com/bitcoin-core/gui/pull/620/commits/67364ebe4c499eb8effe8dac11a5e3648f30c6c7 addressing your suggestions.
hebasto approved
hebasto commented at 10:35 PM on June 21, 2022: member

ACK 67364ebe4c499eb8effe8dac11a5e3648f30c6c7
laanwj commented at 5:11 AM on June 22, 2022: member

Code review and lightly tested ACK 67364ebe4c499eb8effe8dac11a5e3648f30c6c7
laanwj merged this on Jun 22, 2022
laanwj closed this on Jun 22, 2022
sidhujag referenced this in commit 63a9192cd8 on Jun 22, 2022
w0xlt deleted the branch on Jun 23, 2022
bitcoin-core locked this on Jun 23, 2023

Contributors

Labels

Linked (view graph)

#578 Replace `QRegExp` with `QRegularExpression`#606 Replace QRegExp with QRegularExpression