Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace QRegExp with QRegularExpression #620

Merged
merged 3 commits into from
Jun 22, 2022

Conversation

w0xlt
Copy link
Contributor

@w0xlt w0xlt commented Jun 20, 2022

Picking up #606 (labeled "Up for grabs") and applying #606 (review) and #606 (comment).

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

Fixes #578

@hebasto
Copy link
Member

hebasto commented Jun 20, 2022

@w0xlt

Thank you for picking it up!

What is your opinion about #606 (comment)?

@hebasto hebasto changed the title qt: replace QRegExp with QRegularExpression Replace QRegExp with QRegularExpression Jun 20, 2022
@hebasto hebasto added Refactoring Qt 6 Transition to Qt 6 labels Jun 20, 2022
@w0xlt
Copy link
Contributor Author

w0xlt commented Jun 21, 2022

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
Copy link
Member

hebasto commented Jun 21, 2022

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 w0xlt force-pushed the regexp-obsolete branch 2 times, most recently from 39f50ea to c0c8b33 Compare June 21, 2022 15:39
@w0xlt
Copy link
Contributor Author

w0xlt commented Jun 21, 2022

Force-pushed c0c8b33 addressing #606 (comment).

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

Approach ACK c0c8b33.

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.

src/qt/test/optiontests.cpp Outdated Show resolved Hide resolved
src/qt/test/optiontests.cpp Show resolved Hide resolved
src/qt/utilitydialog.cpp Show resolved Hide resolved
src/qt/guiutil.h Outdated Show resolved Hide resolved
w0xlt and others added 3 commits June 21, 2022 19:16
Extract the 'Extract first suffix from filter pattern...'
functionality into a testable utility function
Co-authored-by: Pavol Rusnak <pavol@rusnak.io>
Co-authored-by: Jarol Rodriguez <jarolrod@tutanota.com>
@w0xlt w0xlt force-pushed the regexp-obsolete branch from c0c8b33 to 67364eb Compare June 21, 2022 22:19
@w0xlt
Copy link
Contributor Author

w0xlt commented Jun 21, 2022

@hebasto thanks for the review.
Force-pushed 67364eb addressing your suggestions.

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK 67364eb

@laanwj
Copy link
Member

laanwj commented Jun 22, 2022

Code review and lightly tested ACK 67364eb

@laanwj laanwj merged commit 58b9d6c into bitcoin-core:master Jun 22, 2022
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 22, 2022
@w0xlt w0xlt deleted the regexp-obsolete branch June 23, 2022 11:12
@bitcoin-core bitcoin-core locked and limited conversation to collaborators Jun 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Qt 6 Transition to Qt 6 Refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace QRegExp with QRegularExpression
3 participants