Bug 260586 - JJ: remove QPointer usage, port to QWeakPointer
Summary: JJ: remove QPointer usage, port to QWeakPointer
Status: RESOLVED FIXED
Alias: None
Product: kdevelop
Classification: Applications
Component: general (show other bugs)
Version: git master
Platform: Unlisted Binaries Linux
: NOR minor
Target Milestone: 4.2.0
Assignee: kdevelop-bugs-null
URL:
Keywords: junior-jobs
Depends on:
Blocks:
 
Reported: 2010-12-17 22:19 UTC by Milian Wolff
Modified: 2012-01-18 01:38 UTC (History)
3 users (show)

See Also:
Latest Commit:
Version Fixed In:
Sentry Crash Report:


Attachments
A small patch (6.01 KB, patch)
2010-12-27 09:39 UTC, Nathanaël Restori
Details
The final patch (14.20 KB, patch)
2010-12-27 11:17 UTC, Nathanaël Restori
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Milian Wolff 2010-12-17 22:19:59 UTC
Version:           SVN
OS:                Linux

QPointer is slow and QWeakPointer should be used instead. I've done this now for the background parser as it showed a bottleneck for at least one of our users.

All other usages should be ported as well.

Reproducible: Didn't try
Comment 1 David 2010-12-18 15:16:29 UTC
Is this just a simple "find and replace" QPointer with QWeakPointer? Or are there syntactical differences?
Comment 2 Milian Wolff 2010-12-18 15:48:44 UTC
Most of the time it's just find and replace, but accessing it is not the same:

QPointer can be used as-is, while QWeakPointer must be dereferenced explicitly with .data()

Also this is quite low priority as there are (hopefully) not much more cases where QPointer is used so extensively compared to the BackgroundParser. Still, it would be better so patches welcome.
Comment 3 Nathanaël Restori 2010-12-27 09:39:29 UTC
Created attachment 55280 [details]
A small patch

I've start working on this. Here is a (small) patch who convert QPointer to QWeakPointer in kdevelop/debuggers/gdb/debugsession.{h,cpp}. Tell me if this patch is Ok, and if yes I will continue.
Comment 4 Nathanaël Restori 2010-12-27 11:17:57 UTC
Created attachment 55281 [details]
The final patch

Finally, I converted all QPointer before you answer. This must remove all QPointer from KDevelop.
The sources build but I haven't try to run KDevelop.
Comment 5 Milian Wolff 2010-12-27 14:29:13 UTC
please Nathanael and anyone else interested, please use reviewboard to send in patches:

http://git.reviewboard.kde.org

Some notes on your patch already:

- it shows that in GDB the pointer was used quite a lot without being checked, you should do that at the start of each function, or at least Q_ASSERT on it.
- +    handler_this.clear(); in the ctor doesn't make any sense, just leave it out, the QWeakPointer ctor will init itself properly

and of course we need to test it :) Thanks already
Comment 6 Nathanaël Restori 2010-12-28 19:24:53 UTC
Review request created at http://git.reviewboard.kde.org/r/100252/
Comment 7 Milian Wolff 2011-04-01 10:51:29 UTC
for anybody else: there are still QPointer occurrences in our code base, to find them do e.g.:

git grep QPointer origin master

on either kdevelop or kdevplatform
Comment 8 Glen Kaukola 2011-05-23 00:15:29 UTC
I have a diff up on the review board:
https://git.reviewboard.kde.org/r/101420/

I'm not entirely sure I went about things right, so if someone could have a look.
Comment 9 Glen Kaukola 2011-06-08 06:15:00 UTC
Committed, with the changes suggested from the review board.  I'll try and get to the rest of the QPointer usage here in the next few days.
Comment 10 Glen Kaukola 2011-06-11 22:17:19 UTC
https://git.reviewboard.kde.org/r/101587/
Comment 11 Glen Kaukola 2011-06-12 00:32:07 UTC
https://git.reviewboard.kde.org/r/101588/
Comment 12 Aleix Pol 2012-01-18 01:38:30 UTC
These reviews where committed.