Skip to content

Improve manual implementations of up- and downcasting #1204

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

Merged
merged 19 commits into from
May 19, 2025

Conversation

jnbooth
Copy link
Contributor

@jnbooth jnbooth commented Mar 1, 2025

This PR provides some minor improvements to #1159 for the implementations of Upcast.

  • Replaces downcastPtr's use of dynamic_cast with qobject_cast, which is much faster and does not require RTTI.
  • Implements Upcast<QVector<QPoint>> for QPolygon, replacing the standalone functions previously used for Deref and DerefMut.
  • Implements Upcast<QVector<QPointF>> for QPolygonF, replacing the standalone functions previously used for Deref and DerefMut.
  • Implements Upcast<QCoreApplication> for QGuiApplication and adds Deref. ¹
  • Implements Upcast<QGuiApplication> for QApplication and adds Deref. ¹
  • Implements Deref for QQmlApplicationEngine to QQmlEngine using the existing upcasting implementation. ¹
  • Removes a null pointer created inside QStringList's implementation of Upcast<QList<QString>> due to a misuse of dynamic_cast.

¹ Removed in favor of #1202.

Copy link

codecov bot commented Mar 1, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (fb83a89) to head (1baaae1).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main     #1204   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           73        73           
  Lines        12681     12681           
=========================================
  Hits         12681     12681           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jnbooth jnbooth force-pushed the non-qobject-casting branch 2 times, most recently from d144680 to 6d63031 Compare March 1, 2025 20:42
@jnbooth jnbooth force-pushed the non-qobject-casting branch 2 times, most recently from eeab59b to dee86e9 Compare March 1, 2025 22:07
@jnbooth jnbooth force-pushed the non-qobject-casting branch from dee86e9 to 00886c0 Compare March 1, 2025 22:10
@jnbooth jnbooth force-pushed the non-qobject-casting branch from 00886c0 to 311ddd9 Compare March 1, 2025 22:56
@LeonMatthesKDAB
Copy link
Collaborator

@jnbooth Thank you for the contribution, this is looking good.

Regarding the downcasting, I think we can get all those concerns implemented in a sane way with a bit of meta-programming that automatically selects between qobject_cast and dynamic_cast.

And keeping static_cast for the special cases is okay, though I think we should add another assertion just out of caution.

@jnbooth
Copy link
Contributor Author

jnbooth commented Mar 29, 2025

@LeonMatthesKDAB I don't know how to do that kind of metaprogramming. Are there examples of it in the codebase that I could learn from? Alternatively, should I strip down part of this PR so it can be merged without that?

jnbooth and others added 2 commits March 31, 2025 11:43
Add detection whether to use qobject_cast or dynamic_cast via template
specialization.

Also add additional checks that a static_cast downcast is sane to do
(i.e. at least the size of the types must be the same).
@LeonMatthesKDAB
Copy link
Collaborator

@jnbooth This should do it:
LeonMatthesKDAB@fdd65e9
In case you're interested, google SFINAE and enable_if_t, it's one of the crazier C++ concepts for sure.
But is quite powerful for cases like this.

I pushed this to a branch on my fork (non-qobject-casting) and rebased, feel free to cherry-pick or just use that branch for your PR.

My template meta-programming is a bit rusty nowadays, so @ahayzen-kdab please review carefully to check this is sane.

@jnbooth jnbooth requested a review from ahayzen-kdab April 8, 2025 17:55
@jnbooth
Copy link
Contributor Author

jnbooth commented Apr 18, 2025

Any updates on this? I think at minimum it would be good to remove the null pointer returns.

@jnbooth jnbooth requested a review from LeonMatthesKDAB April 18, 2025 22:48
@LeonMatthesKDAB LeonMatthesKDAB enabled auto-merge (squash) April 28, 2025 08:49
@jnbooth
Copy link
Contributor Author

jnbooth commented May 5, 2025

I don't have approve permissions btw, so it is up to someone else to do that.

@LeonMatthesKDAB
Copy link
Collaborator

Ah sorry @jnbooth this PR probably lost the race with another approval which outdated the auto-merge, this is an annoying issue with our workflow at the moment 🙄
It should auto-merge now after the CI passes 🤞

@LeonMatthesKDAB LeonMatthesKDAB merged commit 752d0f2 into KDAB:main May 19, 2025
16 checks passed
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.

3 participants