Make KoStoreDevice a concrete implementation instead of header only
AbandonedPublic

Authored by vandenoever on Jul 24 2017, 12:27 PM.

Details

Reviewers
dfaure
staniek
Group Reviewers
Calligra: 3.0
Summary

A class derived from QObject should have a cpp file and use Q_OBJECT.
Without that a new class is generated whereever it is used.

Diff Detail

Repository
R8 Calligra
Lint
Lint Skipped
Unit
Unit Tests Skipped
vandenoever created this revision.Jul 24 2017, 12:27 PM
dfaure accepted this revision.Jul 24 2017, 12:44 PM
This revision is now accepted and ready to land.Jul 24 2017, 12:44 PM
staniek requested changes to this revision.Jul 24 2017, 3:07 PM
staniek added a subscriber: staniek.
staniek added inline comments.
libs/store/KoStoreDevice.cpp
22

coding standards

libs/store/KoStoreDevice.h
39

if we're strict, override here would be good too

This revision now requires changes to proceed.Jul 24 2017, 3:07 PM
vandenoever added inline comments.Jul 24 2017, 3:43 PM
libs/store/KoStoreDevice.cpp
22

KoStoreDevice::~KoStoreDevice()
{
}
?

libs/store/KoStoreDevice.h
39

This is not done anywhere in Calligra. I prefer not to start that in this patch.

dfaure added inline comments.Jul 24 2017, 3:47 PM
libs/store/KoStoreDevice.h
39

See also http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rh-override
which advises not to add override on destructors, although it doesn't exactly say why.

staniek added inline comments.Jul 24 2017, 3:50 PM
libs/store/KoStoreDevice.cpp
22

Yep.

libs/store/KoStoreDevice.h
39

True. But KDE's default compiler flags for newer compilers can show many warnings for that. Can be cleaned up (https://www.kdab.com/clang-tidy-part-1-modernize-source-code-using-c11c14/) but I would propose not to add new lines with warnings for new code. This way we reduce number of changes globally and get accustomed with current coding standards.

staniek added inline comments.Jul 24 2017, 3:53 PM
libs/store/KoStoreDevice.h
39

All nice but gcc or clang-tidy is not following this wrt -Wsuggest-override... until then it's academic topic regerding readability :)

vandenoever abandoned this revision.Jul 24 2017, 9:07 PM
vandenoever marked 3 inline comments as done.

I've pushed without the override. I do not feel I understand it well enough to commit it.