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

qAsConst is deprecated, use std::as_const #41

Merged
merged 2 commits into from
Nov 28, 2024
Merged

Conversation

dfaure-kdab
Copy link
Contributor

No description provided.

Copy link

@AndreSomers AndreSomers left a comment

Choose a reason for hiding this comment

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

Looks good to me, but this does mean we're requiring C++17. Is that intentional?

@dfaure-kdab
Copy link
Contributor Author

I couldn't find a KDToolBox-wide CXX_STANDARD requirement, so I don't know what's the current status.
But well who doesn't have C++17 these days?

The bit I do need is UpdateableModel.h, for KDABViewer to build with recent Qt and without deprecation warnings.

@dantti
Copy link
Contributor

dantti commented Nov 22, 2024

I guess it needs to add KDTOOLBOX_CXX17 to disable building and likely requiring it when enabled for it to pass CI

@dfaure-kdab
Copy link
Contributor Author

Qt 6 requires C++17 anyway, but I see the point about Qt5.
Can we make KDTOOLBOX_CXX17 default to ON though, and only disable it on old Qt5+old-compiler CI builds?

@dantti
Copy link
Contributor

dantti commented Nov 22, 2024

It can be on by default, CI compilers are all new enough, it's just that KDStlContainerAdaptor nor Qt5 requires C++-17.

@dfaure-kdab
Copy link
Contributor Author

Hmm, actually instead of disabling entire subdirs for Qt5, I could just do "if qt6 use std::as_const else use qAsConst"

@dfaure-kdab
Copy link
Contributor Author

Done. I reverted the change to qt/qml/PropertySelector/PropertySelector.cpp because that code isn't built anywhere so I can't test changes there => someone else's problem.

@dantti
Copy link
Contributor

dantti commented Nov 22, 2024

LGTM tho on the tests part I think you could've just made the vector const, no?

@dantti
Copy link
Contributor

dantti commented Nov 22, 2024

hmm there are changes to it

@dfaure-kdab
Copy link
Contributor Author

ping? who approves KDToolBox merge requests?

@iamsergio iamsergio self-requested a review November 28, 2024 14:31
@iamsergio iamsergio merged commit 0de6862 into master Nov 28, 2024
7 checks passed
@iamsergio iamsergio deleted the wip/dfaure/as_const branch November 28, 2024 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants