Merge into trunk : add_override_warning : Code : Unity 8

Status:	Merged
Approved by:	Michael Zanetti on 2016-12-01
Approved revision:	2351
Merged at revision:	2728
Proposed branch:	lp:~aacid/unity8/add_override_warning
Merge into:	lp:unity8
Prerequisite:	lp:~aacid/unity8/add_override
Diff against target:	79 lines (+20/-3) 5 files modified CMakeLists.txt (+6/-0) plugins/LightDM/FullLightDM/CMakeLists.txt (+5/-1) plugins/Unity/Indicators/CMakeLists.txt (+3/-0) tests/mocks/QtMultimedia/videooutput.h (+1/-1) tests/uqmlscene/CMakeLists.txt (+5/-1)
To merge this branch:	bzr merge lp:~aacid/unity8/add_override_warning
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Michael Zanetti (community)		2016-05-13	Approve on 2016-12-01
Unity8 CI Bot	continuous-integration		Approve on 2016-11-28
Review via email: mp+294618@code.launchpad.net

Commit Message

Add the Wsuggest-override flag to gcc

While at it mark system includes as such so we don't get warnings we can not fix

Description of the Change

Blocked by https://github.com/ccache/ccache/issues/93

* Are there any related MPs required for this MP to build/function as expected?
Prereq

* Did you perform an exploratory manual test run of your code change and any related functionality?
N/A

* If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?
N/A

* If you changed the UI, has there been a design review?
N/A

Unity8 CI Bot (unity8-ci-bot) wrote on 2016-05-13:

#

PASSED: Continuous integration, rev:2347
https://unity8-jenkins.ubuntu.com/job/lp-unity8-ci/1212/
Executed test runs:
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/test-0-autopkgtest/label=amd64,release=vivid+overlay,testname=qmluitests.sh/753
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/test-0-autopkgtest/label=amd64,release=xenial+overlay,testname=qmluitests.sh/753
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/1627
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/1581
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/1581
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=vivid+overlay/1574
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=vivid+overlay/1574/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/1574
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/1574/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=vivid+overlay/1574
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=vivid+overlay/1574/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/1574
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/1574/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=vivid+overlay/1574
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=vivid+overlay/1574/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/1574
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/1574/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-unity8-ci/1212/rebuild

review: Approve (continuous-integration)

Michael Zanetti (mzanetti) wrote on 2016-05-18:

#

+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

Albert Astals Cid (aacid) wrote on 2016-05-18:

#

ccache is the one to blame :/

http://paste.ubuntu.com/16492473/

Unity8 CI Bot (unity8-ci-bot) wrote on 2016-11-28:

#

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-unity8-ci/2581/rebuild

review: Approve (continuous-integration)

Michael Zanetti (mzanetti) wrote on 2016-12-01:

#

those warnings seem to have disappeared. Most other changes went in already. I'm ok with the remaining changes. It seems to build fine, CI is happy too.

review: Approve

1	=== modified file 'CMakeLists.txt'
2	--- CMakeLists.txt 2016-10-24 11:34:08 +0000
3	+++ CMakeLists.txt 2016-11-28 10:14:25 +0000
4	@@ -40,6 +40,12 @@
5	ENABLE_COVERAGE_REPORT(TARGETS ${SHELL_APP} FILTER /usr/include ${CMAKE_SOURCE_DIR}/tests/* ${CMAKE_BINARY_DIR}/*)
6	endif()
7
8	+if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
9	+ if (NOT CMAKE_CXX_COMPILER_VERSION VERSION_LESS "5.0.0")
10	+ set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wsuggest-override" )
11	+ endif()
12	+endif()
13	+
14	# Make sure we have all the needed symbols
15	set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} -Wl,-z,defs")
16	set(CMAKE_MODULE_LINKER_FLAGS "${CMAKE_MODULE_LINKER_FLAGS} -Wl,-z,defs")
17
18	=== modified file 'plugins/LightDM/FullLightDM/CMakeLists.txt'
19	--- plugins/LightDM/FullLightDM/CMakeLists.txt 2016-07-08 15:44:53 +0000
20	+++ plugins/LightDM/FullLightDM/CMakeLists.txt 2016-11-28 10:14:25 +0000
21	@@ -6,8 +6,12 @@
22
23	include_directories(
24	../
25	+ ${CMAKE_CURRENT_BINARY_DIR}
26	+ )
27	+
28	+include_directories(
29	+ SYSTEM
30	${LIBLIGHTDM_INCLUDE_DIRS}
31	- ${CMAKE_CURRENT_BINARY_DIR}
32	)
33
34	foreach(source_file ${QMLPLUGIN_SRC})
35
36	=== modified file 'plugins/Unity/Indicators/CMakeLists.txt'
37	--- plugins/Unity/Indicators/CMakeLists.txt 2016-06-02 09:32:33 +0000
38	+++ plugins/Unity/Indicators/CMakeLists.txt 2016-11-28 10:14:25 +0000
39	@@ -11,6 +11,9 @@
40	include_directories(
41	SYSTEM
42	${GLIB_INCLUDE_DIRS}
43	+)
44	+include_directories(
45	+ SYSTEM
46	${GIO_INCLUDE_DIRS}
47	${QMENUMODEL_INCLUDE_DIRS}
48	)
49
50	=== modified file 'tests/mocks/QtMultimedia/videooutput.h'
51	--- tests/mocks/QtMultimedia/videooutput.h 2016-05-11 15:58:40 +0000
52	+++ tests/mocks/QtMultimedia/videooutput.h 2016-11-28 10:14:25 +0000
53	@@ -31,7 +31,7 @@
54	QObject *source() const { return m_source.data(); }
55	void setSource(QObject *source);
56
57	- void itemChange(ItemChange change, const ItemChangeData & value);
58	+ void itemChange(ItemChange change, const ItemChangeData & value) override;
59
60	Q_SIGNALS:
61	void sourceChanged();
62
63	=== modified file 'tests/uqmlscene/CMakeLists.txt'
64	--- tests/uqmlscene/CMakeLists.txt 2015-09-17 12:25:29 +0000
65	+++ tests/uqmlscene/CMakeLists.txt 2016-11-28 10:14:25 +0000
66	@@ -9,8 +9,12 @@
67	include_directories(
68	${CMAKE_CURRENT_BINARY_DIR}
69	${CMAKE_SOURCE_DIR}/src
70	+ ${CMAKE_SOURCE_DIR}/libs/UbuntuGestures
71	+)
72	+
73	+include_directories(
74	+ SYSTEM
75	${Qt5Gui_PRIVATE_INCLUDE_DIRS}
76	- ${CMAKE_SOURCE_DIR}/libs/UbuntuGestures
77	)
78
79	if (NOT "${XCB_INCLUDE_DIRS}" STREQUAL "")

Unity 8

Merge lp:~aacid/unity8/add_override_warning into lp:unity8

Commit Message

Description of the Change

Preview Diff

Subscribers