Replace deprecated QWeakPointer::data() with .toStrongRef().data()
AbandonedPublic

Authored by ndavis on Dec 29 2019, 7:20 AM.

Details

Reviewers
None
Group Reviewers
Breeze
Plasma
Summary

As stated in https://doc.qt.io/qt-5/qweakpointer-obsolete.html :
QWeakPointer::data() "Returns the value of the pointer being tracked by this QWeakPointer, without ensuring that it cannot get deleted. To have that guarantee, use toStrongRef(), which returns a QSharedPointer object."

Diff Detail

Repository
R31 Breeze
Branch
replace-deprecated (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 20357
Build 20375: arc lint + arc unit
ndavis created this revision.Dec 29 2019, 7:20 AM
Restricted Application added a project: Plasma. · View Herald TranscriptDec 29 2019, 7:20 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
ndavis requested review of this revision.Dec 29 2019, 7:20 AM

auto c = client().toStrongRef().data();
This is completely wrong, the idea behind shared / weak pointer is ability to know when non-owning resource goes out of scope. by calling toStrongRef you extend its lifetime if and only if it exists at time of call then holding this ref you ensure that resource will not be destroyed meanwhile. By holding data() it does nothing to extend resource lifetime but getting a pointer to well-known dying object. It should be
auto c = m_decoration.data()->client().toStrongRef();
Last but not least, c can be nullptr check against null should be performed as well. Exp

xcb_window_t windowId;
auto c = m_decoration.data()->client().toStrongRef();
if (c && (windowId = c->windowId())) {
} else {
    hide();
}

It should be added checks in all use places.

Thank you for pointing those out. I don't have a lot of experience, so this helps a lot.

-1:
if QWeakPointer::data() is obsoleted, (while needed in the code), I would object to adding toStrongRef, in between, since as pointed out by Antony, calling data immediately after brings no further security, and just results in some overhead (Weak to shared pointer). It means that either

  • weakPointer::data should not be obsoleted
  • the kdecoration api should be changed (to return shared pointers, or provide direct access to the data).

-1:
if QWeakPointer::data() is obsoleted, (while needed in the code), I would object to adding toStrongRef, in between, since as pointed out by Antony, calling data immediately after brings no further security, and just results in some overhead (Weak to shared pointer). It means that either

  • weakPointer::data should not be obsoleted
  • the kdecoration api should be changed (to return shared pointers, or provide direct access to the data).

So this patch is not the correct thing to do at all, even if I make the changes @anthonyfieroni suggested. I doubt getting Qt to undeprecate QWeakPointer::data() is an option, so it seems like changing KDecoration would be the correct thing to do. Does that seem correct to you?

-1:
if QWeakPointer::data() is obsoleted, (while needed in the code), I would object to adding toStrongRef, in between, since as pointed out by Antony, calling data immediately after brings no further security, and just results in some overhead (Weak to shared pointer). It means that either

  • weakPointer::data should not be obsoleted
  • the kdecoration api should be changed (to return shared pointers, or provide direct access to the data).

So this patch is not the correct thing to do at all, even if I make the changes @anthonyfieroni suggested. I doubt getting Qt to undeprecate QWeakPointer::data() is an option, so it seems like changing KDecoration would be the correct thing to do. Does that seem correct to you?

That's my understanding yes. As long as access to the true data is needed (here for signal/slot connections) by the customers of kdecoration, then kdecoration must provide this access, without roundtrips to shared pointers.
Others please correct me if I am wrong.

ndavis abandoned this revision.Dec 30 2019, 1:03 AM