refactor: fix various compiler warnings
ClosedPublic

Authored by dvratil on Feb 24 2020, 3:03 PM.

Details

Summary

Fixes pessimizing move, implicit copy assignment operator, copies
in range for loops, deprecated Qt methods, missing final keyword
and unused lambda capture.

Built with Clang 10

Diff Detail

Repository
R104 KScreen
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 22858
Build 22876: arc lint + arc unit
dvratil created this revision.Feb 24 2020, 3:03 PM
Restricted Application added a project: Plasma. · View Herald TranscriptFeb 24 2020, 3:03 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
dvratil requested review of this revision.Feb 24 2020, 3:03 PM
romangg added inline comments.
kcm/output_model.h
90

Why to add and why noexcept?

dvratil added inline comments.Feb 25 2020, 1:18 PM
kcm/output_model.h
90

Because in output_model.cpp:224 we do m_outputs.insert(i, Output(output, pos)), which can make use of move semantics.

Making move operations noexcept is part of Cpp Core Guidelines and because QSharedPointer's move ctor is noexcept and QPoint is trivially movable, the move operations here are also noexcept.

If it has any benefit here, since we possibly don't even have exceptions enabled in kscreen, is a question - I wrote it by force of habit.

romangg accepted this revision.Feb 25 2020, 2:42 PM
This revision is now accepted and ready to land.Feb 25 2020, 2:42 PM
romangg added inline comments.Feb 25 2020, 2:47 PM
kcm/output_model.h
90

Thanks for the interesting explanation. I find it impractical that the usefulness of the noexcept keyword here depends on internals of the Qt classes. One would wish this is advertised to the consumer without knowing the internals. Or is it somewhere I'm not aware of?

A good rule of a thumb is that your move constructor and assignment operator should always be noexcept unless you have a very good reason for them not to be. Qt has most move constructors marked as noexcept - I found a few that are not marked but that looks more like an oversight rather than intent, more so that Qt does not use exceptions - so if you have a type composed Qt types, making the constructor noexcept is perfectly safe. For custom types, even if their move constructors/assignment operators are not marked as noexcept it's fairly safe to assume they do not throw since we generally do not use exceptions in KDE.

Also note that noexcept is not a compile-time check, it's a hint to enable some optimizations. It makes a difference when using standard library containers, e.g. std::vector<T> will perform a copy if T's move constructor is not noexcept in order to guarantee strong exceptions safety.

This revision was automatically updated to reflect the committed changes.