[GTK][CMake] Add support for building with Clang
Created attachment 226761 [details] Patch
Comment on attachment 226761 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=226761&action=review > Source/cmake/OptionsCommon.cmake:37 > + set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -stdlib=libc++ -Qunused-arguments") CMake + gcc issues warnings about unused arguments, so I don't think we want this to be different with clang.
Comment on attachment 226761 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=226761&action=review >> Source/cmake/OptionsCommon.cmake:37 >> + set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -stdlib=libc++ -Qunused-arguments") > > CMake + gcc issues warnings about unused arguments, so I don't think we want this to be different with clang. I believe you're thinking of -Wunused-arguments. -Qunused-arguments suppresses any arguments that were unused by the Clang driver, not unused arguments in code.
Comment on attachment 226761 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=226761&action=review > Source/cmake/OptionsCommon.cmake:37 > + set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -stdlib=libc++ -Qunused-arguments") Is there a particular reason for forcing libc++?
(In reply to comment #4) > (From update of attachment 226761 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=226761&action=review > > > Source/cmake/OptionsCommon.cmake:37 > > + set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -stdlib=libc++ -Qunused-arguments") > > Is there a particular reason for forcing libc++? It's best optimized for Clang much like libstdc++ is best optimized for GCC. For instance, when benchmarking changes in bug #128995 Clang with libc++ gave better numbers than Clang with libstdc++. The current WebKit code also has a few tender places where build errors occur when using Clang and libstdc++ 4.7 or 4.8.0 (but not 4.8.1 which is C++11-complete). There are no macro-based mechanisms that are oriented towards differentiating the code for the specific standard library in use, unlike the ones that enable guarding compiler-specific code.
(In reply to comment #5) > (In reply to comment #4) > > Is there a particular reason for forcing libc++? > > It's best optimized for Clang much like libstdc++ is best optimized for GCC. For instance, when benchmarking changes in bug #128995 Clang with libc++ gave better numbers than Clang with libstdc++. Still, forcing a certain standard library is a problem for downstreams: not all people who may want to try clang also have libc++ installed, and even if they do but libstdc++ is used by default there are going to be crashes if people then mix the WebKit library(ies) with the rest of their code (ie. they produce a binary that depends on both libstdc++ and libc++, which has been a source of many crashes on FreeBSD land, for example). I believe it would be more beneficial and less likely to cause headaches if users were just made aware of the performance differences caused by using different standard libraries with clang. > The current WebKit code also has a few tender places where build errors occur when using Clang and libstdc++ 4.7 or 4.8.0 (but not 4.8.1 which is C++11-complete). There are no macro-based mechanisms that are oriented towards differentiating the code for the specific standard library in use, unlike the ones that enable guarding compiler-specific code. The usual idiom (for checking for libc++ at least) is to include a harmless header (for example, ciso646) and check for _LIBCPP_VERSION. See http://trac.webkit.org/changeset/108174 or http://trac.webkit.org/changeset/108179, for example.
OK, let's leave it up to Clang to decide what standard library it wants to use. Inspecting the 3.4 release it looks like libstdc++ is chosen before libc++ for most toolchains (including Linux), with at least FreeBSD and NetBSD being exceptions. libc++ will still be enforceable through CXXFLAGS, of course. (In reply to comment #6) > > The current WebKit code also has a few tender places where build errors occur when using Clang and libstdc++ 4.7 or 4.8.0 (but not 4.8.1 which is C++11-complete). There are no macro-based mechanisms that are oriented towards differentiating the code for the specific standard library in use, unlike the ones that enable guarding compiler-specific code. > > The usual idiom (for checking for libc++ at least) is to include a harmless header (for example, ciso646) and check for _LIBCPP_VERSION. See http://trac.webkit.org/changeset/108174 or http://trac.webkit.org/changeset/108179, for example. I'll try to introduce a configure-time check for the Clang and libstdc++ combo, similar to one used in Autotools.
Created attachment 226933 [details] Patch
Comment on attachment 226933 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=226933&action=review > Source/cmake/gtest/CMakeLists.txt:27 > +if ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "Clang") Just a nitpick/suggestion: newer versions of gtest have proper support for libc++, so maybe it would be good to add a comment indicating this could be removed once gtest is updated?
Comment on attachment 226933 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=226933&action=review >> Source/cmake/gtest/CMakeLists.txt:27 >> +if ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "Clang") > > Just a nitpick/suggestion: newer versions of gtest have proper support for libc++, so maybe it would be good to add a comment indicating this could be removed once gtest is updated? Sure, I'll add it in the next iteration or before landing.
Comment on attachment 226933 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=226933&action=review > Source/cmake/OptionsCommon.cmake:40 > + set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Qunused-arguments") > + set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Qunused-arguments") > +endif () Small nit. There's an extra space after FLAGS in both of these.
Committed r165975: <http://trac.webkit.org/changeset/165975>