Enable additional compiler warnings for CMake build #283

dgrove-oss · 2017-07-25T20:28:46Z

add a CMake module to define additional warning flags to enable
when compiling dispatch.
enable the additional warning flags by default for CMake
match autotools behavior of not including the BSD_OVERLAY
when compiling the dispatch_c99 test case. This avoids
a warning about __printflike being redefined when compiling
this test case with the expanded set of warning flags.

dgrove-oss · 2017-07-25T20:29:55Z

@compnerd please comment if there are better ways to do this in CMake. Thanks

compnerd · 2017-07-26T01:19:52Z

cmake/modules/DispatchCompilerWarnings.cmake

@@ -0,0 +1,85 @@
+
+macro(dispatch_common_warnings)


This definitely is problematic. You break cross-compiling for Windows this way. You should check if the compiler flag is supported and if you are using a GNU frontend before adding these options. This is also a very large set of flags that you are adding, maintaining this seems like a headache. How were these handled in autotools?

these are the flags we have enabled in the darwin build internally (in Xcode, see the .xcconfig files in the repo), so shared code should be built with these enabled to avoid breaking the darwin build (and I want the darwin CMake build to definitely have these so that it matches the Xcode build as closely as possible).
We maintain these flags internally by hand by adding or removing new ones as clang adds support for them (based on -Weverything). Once the opensource buildsystem matches, we can start maintaining these in parallel.

If we need to opt out non-clang platforms like Windows that we don't build & support on a continual basis right now, that is a much lesser concern IMO

compnerd · 2017-07-26T03:31:01Z

tests/CMakeLists.txt

@@ -91,7 +91,7 @@ function(add_unit_test name)
  if(WITH_BLOCKS_RUNTIME)
    target_link_libraries(${name} PRIVATE BlocksRuntime)
  endif()
-  if(BSD_OVERLAY_FOUND)
+  if(BSD_OVERLAY_FOUND AND (NOT ${name} MATCHES "dispatch_c99"))


These exclusions are ugly. How about adding a NO_BSD_OVERLAY keyword option to the add_unit_test so that it is generic.

dgrove-oss · 2017-07-26T17:53:32Z

updated based on review comments.

compnerd · 2017-07-27T01:56:38Z

cmake/modules/DispatchCompilerWarnings.cmake

@@ -0,0 +1,91 @@
+
+# TODO: really should be checking for clang/gcc compatibile compiler 
+if(CMAKE_SYSTEM_NAME STREQUAL Windows)


if("${CMAKE_C_SIMULATE_ID}" STREQUAL "MSVC")

compnerd · 2017-07-27T01:57:58Z

tests/CMakeLists.txt

@@ -58,7 +58,7 @@ if(ENABLE_SWIFT)
                          swiftSwiftOnoneSupport)
 endif()

-function(add_unit_test name)
+function(add_unit_test name uses_bsd_overlay)


Just make this a keyword argument (add NO_BSD_OVERLAY to the options list below), you can reference it as AUT_NO_BSD_OVERLAY then.

dgrove-oss · 2017-07-27T12:48:22Z

updated again based on @compnerd feedback

compnerd · 2017-07-28T01:32:05Z

tests/CMakeLists.txt

@@ -60,6 +60,7 @@ endif()

 function(add_unit_test name)
  set(options DISABLED_TEST)
+  set(options NO_BSD_OVERLAY)


This drops the DISABLED_TEST since you overwrite the variable. Just append to it.

set(options DISABLED_TEST;NO_BSD_OVERLAY)

compnerd · 2017-07-28T01:32:33Z

tests/CMakeLists.txt

@@ -138,6 +138,8 @@ set(DISPATCH_C_TESTS
    data
    io_net
    select)
+set(DISPATCH_C99_TESTS


I wouldn't bother with a list for a single test. Just inline it below.

compnerd

We shouldn't drop the DISABLED_TEST option.

dgrove-oss · 2017-07-28T14:21:39Z

updated.

There are probably going to be merge conflicts between this and #284. I'd suggest merging this one first and I will update #284 to resolve the conflicts after this is merged to master.

compnerd

This looks great! Im still not a fan of the management of the warnings, but if everyone else is okay with that, lets go with it.

das · 2017-07-28T18:34:39Z

@dgrove-oss sounds good, I've merged #284

das · 2017-07-28T18:35:30Z

argh, and now I re-read what you said and I did it the wrong way around... sorry!

dgrove-oss · 2017-07-28T18:52:52Z

no worries; I can make it work either way. working on it now

1. add a CMake module to define additional warning flags to enable when compiling dispatch. 2. enable the additional warning flags by default for CMake 3. match autotools behavior of not including the BSD_OVERLAY when compiling the dispatch_c99 test case. This avoids a warning about __printflike being redefined when compiling this test case with the expanded set of warning flags.

dgrove-oss · 2017-07-28T19:23:02Z

ok. conflicts resolved. should be ready to go.

das · 2017-07-29T00:57:46Z

thanks!

Enable additional compiler warnings for CMake build Signed-off-by: Daniel A. Steffen <dsteffen@apple.com>

compnerd reviewed Jul 26, 2017

View reviewed changes

dgrove-oss force-pushed the cmake-extended-warnings branch from cffdf51 to 881106d Compare July 26, 2017 17:50

compnerd reviewed Jul 27, 2017

View reviewed changes

dgrove-oss force-pushed the cmake-extended-warnings branch from 881106d to d120813 Compare July 27, 2017 12:46

compnerd reviewed Jul 28, 2017

View reviewed changes

compnerd requested changes Jul 28, 2017

View reviewed changes

dgrove-oss force-pushed the cmake-extended-warnings branch from d120813 to 9babf1b Compare July 28, 2017 14:19

compnerd approved these changes Jul 28, 2017

View reviewed changes

das added the darwin pending label Jul 28, 2017

dgrove-oss force-pushed the cmake-extended-warnings branch from 9babf1b to 0652ef8 Compare July 28, 2017 19:21

das merged commit 291f34d into swiftlang:master Jul 29, 2017

dgrove-oss deleted the cmake-extended-warnings branch July 29, 2017 17:51

ktopley-apple pushed a commit that referenced this pull request Dec 6, 2018

Merge pull request #283 from dgrove-oss/cmake-extended-warnings

69422a9

Enable additional compiler warnings for CMake build Signed-off-by: Daniel A. Steffen <dsteffen@apple.com>

ktopley-apple removed the darwin pending label Apr 5, 2019

		@@ -0,0 +1,91 @@

		# TODO: really should be checking for clang/gcc compatibile compiler
		if(CMAKE_SYSTEM_NAME STREQUAL Windows)

Enable additional compiler warnings for CMake build #283

Enable additional compiler warnings for CMake build #283

Uh oh!

Conversation

dgrove-oss commented Jul 25, 2017

Uh oh!

dgrove-oss commented Jul 25, 2017

Uh oh!

compnerd Jul 26, 2017

Choose a reason for hiding this comment

Uh oh!

das Jul 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

compnerd Jul 26, 2017

Choose a reason for hiding this comment

Uh oh!

dgrove-oss commented Jul 26, 2017

Uh oh!

compnerd Jul 27, 2017

Choose a reason for hiding this comment

Uh oh!

compnerd Jul 27, 2017

Choose a reason for hiding this comment

Uh oh!

dgrove-oss commented Jul 27, 2017

Uh oh!

compnerd Jul 28, 2017

Choose a reason for hiding this comment

Uh oh!

compnerd Jul 28, 2017

Choose a reason for hiding this comment

Uh oh!

compnerd left a comment

Choose a reason for hiding this comment

Uh oh!

dgrove-oss commented Jul 28, 2017

Uh oh!

compnerd left a comment

Choose a reason for hiding this comment

Uh oh!

das commented Jul 28, 2017

Uh oh!

das commented Jul 28, 2017

Uh oh!

dgrove-oss commented Jul 28, 2017

Uh oh!

dgrove-oss commented Jul 28, 2017

Uh oh!

das commented Jul 29, 2017

Uh oh!

Uh oh!

das Jul 26, 2017 •

edited

Loading