Some other ports use -Werror as well, but the warning level between ports are not the same, so we should consider which warnings should be treated as errors (assumption is that most warnings should be treated as errors).
Created attachment 167682 [details] proposed patch Treat all warnings as errors, except the "unused parameter" warning. The "unused parameter" warning is not turned on for other ports as warning, so that will require some discussion with the other ports. Removed the ANOTHER_BRICK_IN_THE define as I do not think that is used (I found it confusing to have the define in the build logs passed on CLI to the compiler).
Comment on attachment 167682 [details] proposed patch As we talked about this yesterday, I agree with this patch. IMO, it would be good if you notify this to webkit-efl mailing list.
Looks good. Thanks.
Comment on attachment 167682 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=167682&action=review > ChangeLog:3 > + [EFL] Enable -Werror for the EFL port BTW, Is this patch adjusted into other ports which uses cmake ? I think [CMAKE] prefix is proper instead of [EFL].
(In reply to comment #1) > Removed the ANOTHER_BRICK_IN_THE define as I do not think that is used (I found it confusing to have the define in the build logs passed on CLI to the compiler). It was a nice easter egg :}
(In reply to comment #4) > BTW, Is this patch adjusted into other ports which uses cmake ? I think [CMAKE] prefix is proper instead of [EFL]. This is a great point. I think it would be difficult to coordinate flipping the Werror flag for all cmake-based ports at the same time, so I really only meant to do it for the EFL port first. I will upload a new patch.
Created attachment 167902 [details] make the change for the EFL port only and do nto change the behavior for other cmake-based ports
Comment on attachment 167902 [details] make the change for the EFL port only and do nto change the behavior for other cmake-based ports Looks fine.
Instead of making this change opt-out, I would make it opt-in: we already have the code in the WEBKIT_SET_EXTRA_COMPILER_FLAGS() to make it accept additional options. You could just accept an extra option there which would flip the -Werror switch.
Created attachment 172619 [details] Make it opt in instead of opt-out (see comment #9) I also had to exclude more warnings to be not fire errors to make sure it builds.
Comment on attachment 172619 [details] Make it opt in instead of opt-out (see comment #9) Attachment 172619 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14731838
Created attachment 172650 [details] adding -Wno-error=unused-function to list of flags one more exclusion to make sure it builds.
Comment on attachment 172650 [details] adding -Wno-error=unused-function to list of flags My suggestion still holds: instead of using a global variable like that, accept another option in the WEBKIT_SET_EXTRA_COMPILER_FLAGS macro and parse it just like we already do with IGNORECXX_WARNINGS.
Comment on attachment 172650 [details] adding -Wno-error=unused-function to list of flags View in context: https://bugs.webkit.org/attachment.cgi?id=172650&action=review > Source/cmake/WebKitHelpers.cmake:-40 > - SET(OLD_COMPILE_FLAGS "-DANOTHER_BRICK_IN_THE -Wall -Wextra -Wcast-align -Wformat-security -Wmissing-format-attribute -Wpointer-arith -Wundef -Wwrite-strings ${OLD_COMPILE_FLAGS}") -DANOTHER_BRICK_IN_THE -Wall is a long standing joke on the CMake buildsystem, I think it should stay. > Source/cmake/WebKitHelpers.cmake:45 > + SET(OLD_COMPILE_FLAGS "-Werror -Wno-error=unused-parameter -Wno-error=switch -Wno-error=sign-compare -Wno-error=unused-function ${OLD_COMPILE_FLAGS}") I would write this as SET(OLD_COMPILE_FLAGS ${OLD_COMPILE_FLAGS} ...) to make it explicit that you are appending parameters.
Comment on attachment 172650 [details] adding -Wno-error=unused-function to list of flags View in context: https://bugs.webkit.org/attachment.cgi?id=172650&action=review >> Source/cmake/WebKitHelpers.cmake:-40 >> - SET(OLD_COMPILE_FLAGS "-DANOTHER_BRICK_IN_THE -Wall -Wextra -Wcast-align -Wformat-security -Wmissing-format-attribute -Wpointer-arith -Wundef -Wwrite-strings ${OLD_COMPILE_FLAGS}") > > -DANOTHER_BRICK_IN_THE -Wall is a long standing joke on the CMake buildsystem, I think it should stay. I don't see the point in keeping this "joke". It makes the line uselessly longer. If anything it does not look serious.
Comment on attachment 172650 [details] adding -Wno-error=unused-function to list of flags updated patch coming..
Created attachment 172814 [details] take rakuco's feedback
Comment on attachment 172814 [details] take rakuco's feedback Attachment 172814 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14760424
Created attachment 172849 [details] one more try Add back -Wno-error=switch for now and do not (yet) add Werror for the WebKit2 sources to fix the build. After this patch landed I will file separate bugs to resolve these issues.
LGTM if the EWS does not complain.
Comment on attachment 172849 [details] one more try Attachment 172849 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14766201
Created attachment 172892 [details] one more try hmm.. I have not had this build failure locally. Excluding webkit1 sources from Werror as well for now..
Doesn't it sense to fix the WebKit bits before landing this one?
(In reply to comment #23) > Doesn't it sense to fix the WebKit bits before landing this one? I prefer to land the baseline first and start fixing the remaining issues. We normally take the same approach with Layouttests as well. When I first introduce this patch about a month ago I only had to exclude unused-parameter. I month later it seems I had to exclude unused-parameter, sign-compare, switch and WebKit1 WebKit2. I think the longer we chase the trunk the harder it is to do it in one patch.
Comment on attachment 172892 [details] one more try Clearing flags on attachment: 172892 Committed r134057: <http://trac.webkit.org/changeset/134057>
All reviewed patches have been landed. Closing bug.