Don't crash with Qt 5.11+
ClosedPublic

Authored by amantia on Dec 13 2018, 3:33 PM.

Diff Detail

Repository
R201 Akregator
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 6002
Build 6020: arc lint + arc unit
amantia created this revision.Dec 13 2018, 3:33 PM
Restricted Application added a project: KDE PIM. · View Herald TranscriptDec 13 2018, 3:33 PM
Restricted Application added a subscriber: kde-pim. · View Herald Transcript
amantia requested review of this revision.Dec 13 2018, 3:33 PM
kfunk added a subscriber: kfunk.Dec 13 2018, 4:49 PM
kfunk added inline comments.
src/articleviewer-ng/webengine/articleviewerwebengine.cpp
82

Note: You dont actually need a member at all here, no?

amantia added inline comments.Dec 13 2018, 6:28 PM
src/articleviewer-ng/webengine/articleviewerwebengine.cpp
82

Right, it is possible without it as well.

amantia updated this revision to Diff 47536.Dec 13 2018, 6:31 PM

Removed member var

amantia updated this revision to Diff 47537.Dec 13 2018, 6:32 PM

Add override

mlaurent added inline comments.Dec 14 2018, 6:30 AM
src/articleviewer-ng/webengine/articleviewerwebengine.cpp
68

~AkregatorRequestInterceptor() override = default(); no ?

marten added a subscriber: marten.Dec 14 2018, 7:50 AM
marten added inline comments.
src/articleviewer-ng/webengine/articleviewerwebengine.cpp
68

Is there any point in using 'override' on destructors? There is no function signature to trigger the usual warning case (derived class defines a function with a different signature, so not really replacing the base class virtual), so there is no way a destructor can be written to not override. The compiler doesn't consider destructors in its override warning.

The only way in which 'override' would be useful on a destructor would be to give an error if the base class destructor was not declared virtual, but with a correctly written base class this should not arise.

amantia updated this revision to Diff 47548.Dec 14 2018, 8:08 AM

Remove unneeded destructor

amantia added inline comments.Dec 14 2018, 8:09 AM
src/articleviewer-ng/webengine/articleviewerwebengine.cpp
68

I removed the destructor completely.

kfunk added inline comments.Dec 14 2018, 8:26 AM
src/articleviewer-ng/webengine/articleviewerwebengine.cpp
68

Sorry for getting more off-topic, but /me wants to clarify this:

The only way in which 'override' would be useful on a destructor would be to give an error if the base class destructor was not declared virtual, (...)

Right, and why not protect against this? I think adding an override to dtors makes a lot of sense. Especially if you think of override as of: "I expect there to exist a base class version of this function which is declared virtual".

Also see discussion on qt-development: https://lists.qt-project.org/pipermail/development/2018-August/033437.html

amantia updated this revision to Diff 47595.Dec 14 2018, 9:44 PM

Simplify and fix (hopefully) one of the random crash on exit in Kontact.

kfunk added a comment.Dec 15 2018, 8:48 AM

FYI: Patch looks good to me now, but I cannot judge whether it fixed the crash. So cannot give a +1.

All I can say that I use it since I've created the patch and Akregator never crashed when closing a tab. :)

mlaurent accepted this revision.Dec 15 2018, 10:00 AM

I can't judge that it fixes your crash.
but +2

This revision is now accepted and ready to land.Dec 15 2018, 10:00 AM
amantia closed this revision.Dec 15 2018, 11:59 AM