Code review comment for lp:~aacid/unity8/add_override_warning

Michael Zanetti (mzanetti) wrote :

+1 on removing the pragmas and using the SYSTEM keyword on the includes instead.

In general I would be for enabling -Wsuggest-override, however, it triggers a lot of warnings that we might not be able to fix, like

In file included from /home/micha/Develop/reviews/add_override_warning/builddir/tests/mocks/Unity/moc_fake_settingsmodel.cpp:9:0,
                 from /home/micha/Develop/reviews/add_override_warning/builddir/tests/mocks/Unity/FakeUnityQml_automoc.cpp:15:
/home/micha/Develop/reviews/add_override_warning/builddir/tests/mocks/Unity/../../../../tests/mocks/Unity/fake_settingsmodel.h:26:72: warning: ‘virtual const QMetaObject* SettingsModel::metaObject() const’ can be marked override [-Wsuggest-override]
/home/micha/Develop/reviews/add_override_warning/builddir/tests/mocks/Unity/../../../../tests/mocks/Unity/fake_settingsmodel.h:26:106: warning: ‘virtual void* SettingsModel::qt_metacast(const char*)’ can be marked override [-Wsuggest-override]
/home/micha/Develop/reviews/add_override_warning/builddir/tests/mocks/Unity/../../../../tests/mocks/Unity/fake_settingsmodel.h:26:145: warning: ‘virtual int SettingsModel::qt_metacall(QMetaObject::Call, int, void**)’ can be marked override [-Wsuggest-override]

Would it be possible to exclude mocked code from it? Also it seems the Q_OBJECT macro generates some code that fails this check. In order to enable this we'd need to solve those problems, otherwise we'll have so much noise on the compiler output that proper warnings would drown.

review: Needs Information

« Back to merge proposal