-
Notifications
You must be signed in to change notification settings - Fork 99
Support both Qt 5 and Qt 6 at the same time (fixes #163) #165
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
Conversation
For unconditionally using |
@a17r thanks for the review and for bringing this up. I confirm 5.14 to be needed for… …and made the QMake project file reject Qt <5.14.0 now. It seems like the only changes not guarded behind >=6.0.0 checks so I'm leaning towards 5.14 rather than 5.15 so far. Pushed. |
I've not used C++ or Qt in quite a few years, but with that caveat out of the way... Looks good to me. |
I think it could be simpler in a couple of steps. |
@tnyblom @svuorela thanks for the review! @svuorela I confirm that (1) not supporting both Qt 5 and Qt 6 at the same time would be simpler and (2) that Qt 5 does have |
@svuorela PS: With regard to |
I'm just a driveby reviewer, so don't get too much riled up about my ideas. Maybe just a and then just use MapIterator everywhere... |
@svuorela either I misunderstand the idea or you want to use |
Fixes #163
Please note that:
QRegExp
because successorQRegularExpression
has a different pattern syntax and so backwards compatibility would be broken e.g. with regard to existing rules files. That migration should probably be a distinct future pull request and come with a bump in major version to signal its breaking nature.