By compiling ANGLE into a static library (like wtf.a) we can set our compile rules for these files (partly C code) and so suppress warnings that we see now on ToT. One example of a warning that is hard to fix is the usage of -Wno-c++0x-compat on a C source.
Created attachment 161314 [details] Patch
Comment on attachment 161314 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161314&action=review > Source/WebCore/CMakeLists.txt:2970 > + SET_TARGET_PROPERTIES (${ANGLESupport_LIBRARY_NAME} PROPERTIES COMPILE_FLAGS "-fPIC -fvisibility=hidden -fno-exceptions -fno-strict-aliasing") Doesn't this cause problems if one's not using gcc/clang?
(In reply to comment #0) > By compiling ANGLE into a static library (like wtf.a) we can set our compile rules for these files (partly C code) and so suppress warnings that we see now on ToT. One example of a warning that is hard to fix is the usage of -Wno-c++0x-compat on a C source. Do you mean some of the .cpp files there are actually C source code?
(In reply to comment #3) > (In reply to comment #0) > > By compiling ANGLE into a static library (like wtf.a) we can set our compile rules for these files (partly C code) and so suppress warnings that we see now on ToT. One example of a warning that is hard to fix is the usage of -Wno-c++0x-compat on a C source. > > Do you mean some of the .cpp files there are actually C source code? I meant, ANGLE contain C sources, so using -Wno-c++0x-compat on them gives a warning. An example is ${THIRDPARTY_DIR}/ANGLE/src/compiler/preprocessor/atom.c.
(In reply to comment #3) > (In reply to comment #0) > > By compiling ANGLE into a static library (like wtf.a) we can set our compile rules for these files (partly C code) and so suppress warnings that we see now on ToT. One example of a warning that is hard to fix is the usage of -Wno-c++0x-compat on a C source. > > Do you mean some of the .cpp files there are actually C source code? (In reply to comment #2) > (From update of attachment 161314 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=161314&action=review > > > Source/WebCore/CMakeLists.txt:2970 > > + SET_TARGET_PROPERTIES (${ANGLESupport_LIBRARY_NAME} PROPERTIES COMPILE_FLAGS "-fPIC -fvisibility=hidden -fno-exceptions -fno-strict-aliasing") > > Doesn't this cause problems if one's not using gcc/clang? Possibly yes. I'll look into that tomorrow, thanks.
(In reply to comment #4) > (In reply to comment #3) > > (In reply to comment #0) > > > By compiling ANGLE into a static library (like wtf.a) we can set our compile rules for these files (partly C code) and so suppress warnings that we see now on ToT. One example of a warning that is hard to fix is the usage of -Wno-c++0x-compat on a C source. > > > > Do you mean some of the .cpp files there are actually C source code? > > I meant, ANGLE contain C sources, so using -Wno-c++0x-compat on them gives a warning. An example is ${THIRDPARTY_DIR}/ANGLE/src/compiler/preprocessor/atom.c. Oh, I see. That makes sense; perhaps we should not set the target's COMPILE_FLAGS in WebKitHelper.cmake's WEBKIT_SET_EXTRA_COMPILER_FLAGS and just change CMAKE_C{XX}_FLAGS instead. Splitting ANGLE into a separate library is a good change regardless of that, though.
Created attachment 161490 [details] Patch
Comment on attachment 161490 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161490&action=review > Source/cmake/WebKitHelpers.cmake:7 > + IF (${ARGC} MATCHES "1") > + SET (_allowCXXWarnings "allow") > + ENDIF () Now that we require CMake 2.8.3, we can parse keyword arguments more easily. I recommend taking a look at the CMakeParseArguments module, we could then accept something like WEBKIT_SET_EXTRA_COMPILER_FLAGS(foo IGNORE_WARNINGS FALSE).
(In reply to comment #8) > we could then accept something like WEBKIT_SET_EXTRA_COMPILER_FLAGS(foo IGNORE_WARNINGS FALSE). ... Or even WEBKIT_SET_EXTRA_COMPILER_FLAGS(foo IGNORE_WARNINGS), which is more compact and makes more sense.
(In reply to comment #9) > (In reply to comment #8) > > we could then accept something like WEBKIT_SET_EXTRA_COMPILER_FLAGS(foo IGNORE_WARNINGS FALSE). > > ... Or even WEBKIT_SET_EXTRA_COMPILER_FLAGS(foo IGNORE_WARNINGS), which is more compact and makes more sense. I agree, this sounds like a lot cleaner way to do it, thanks. Patch upcoming.
Created attachment 161521 [details] Patch
Comment on attachment 161521 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161521&action=review > Source/cmake/WebKitHelpers.cmake:4 > + SET(options IGNORECXX_WARNINGS) Nit: Since there's only one argument right now it could be simpler to just pass it directly to CMAKE_PARSE_ARGUMENTS() instead of setting a variable for that. > Source/cmake/WebKitHelpers.cmake:5 > + CMAKE_PARSE_ARGUMENTS("OPTION" "${options}" "" "" ${ARGN} ) You likely need to INCLUDE(CMakeParseArguments), for example at the beginning of the file, before using this macro. One minor nit is that there's an extra space character before the closing ')'.
Created attachment 161528 [details] Patch
Comment on attachment 161528 [details] Patch rs=me
Committed r127178: <http://trac.webkit.org/changeset/127178>