Bug 95377 – [CMake] Suppress ANGLE compilation warnings

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Rob Buis]

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.

[Rob Buis]

Created attachment 161314 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

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?

[Raphael Kubo da Costa (:rakuco)]

(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?

[Rob Buis]

(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.

[Rob Buis]

(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.

[Raphael Kubo da Costa (:rakuco)]

(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.

[Rob Buis]

Created attachment 161490 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

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).

[Raphael Kubo da Costa (:rakuco)]

(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.

[Rob Buis]

(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.

[Rob Buis]

Created attachment 161521 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

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 ')'.

[Rob Buis]

Created attachment 161528 [details]
Patch

[Antonio Gomes]

Comment on attachment 161528 [details]
Patch

rs=me

[Rob Buis]

Committed r127178: <http://trac.webkit.org/changeset/127178>