[CMake] Change the WARNINGS_AS_ERRORS option from BOOL to STRING

to allow a new mode `SERIOUS_ONLY`.

Modes:

`ON` - All warnings are treated as errors (same as before)
`OFF` - Warnings are not treated as errors (same as before)
`SERIOUS_ONLY` - A subset of "serious" warnings are treated as errors.

Upgrade code is included to upgrade old CMake cache's to use the new
type of `WARNINGS_AS_ERRORS`. We should remove it eventually. The
user's previous setting is preserved when doing this.

Very few warnings are treated as errors for now. Developers can
add more later as they see fit.

Author: Dan Liew

README-CMake.md

@@ -266,6 +266,8 @@ The following useful options can be passed to CMake whilst configuring.
     Disabling this is useful for faster incremental builds. The documentation can be manually built by invoking the ``api_docs`` target.
 * ``LINK_TIME_OPTIMIZATION`` - BOOL. If set to ``TRUE`` link time optimization will be enabled.
 * ``API_LOG_SYNC`` - BOOL. If set to ``TRUE`` will enable experimental API log sync feature.
+* ``WARNINGS_AS_ERRORS`` - STRING. If set to ``TRUE`` compiler warnings will be treated as errors. If set to ``False`` compiler warnings will not be treated as errors.
+    If set to ``SERIOUS_ONLY`` a subset of compiler warnings will be treated as errors.
 
 On the command line these can be passed to ``cmake`` using the ``-D`` option. In ``ccmake`` and ``cmake-gui`` these can be set in the user interface.
 
@@ -381,3 +383,13 @@ It is tempting use file-globbing in ``CMakeLists.txt`` to find a set for files m
 use them as the sources to build a target. This however is a bad idea because it prevents CMake from knowing when it needs to rerun itself. This is why source file names are explicitly listed in the ``CMakeLists.txt`` so that when changes are made the source files used to build a target automatically triggers a rerun of CMake.
 
 Long story short. Don't use file globbing.
+
+### Serious warning flags
+
+By default the `WARNINGS_AS_ERRORS` flag is set to `SERIOUS_ONLY` which means
+some warnings will be treated as errors. These warnings are controlled by the
+relevant `*_WARNINGS_AS_ERRORS` list defined in
+`cmake/compiler_warnings.cmake`.
+
+Additional warnings should only be added here if the warnings has no false
+positives.

cmake/compiler_warnings.cmake

@@ -1,17 +1,61 @@
+################################################################################
+# Compiler warning flags
+################################################################################
+# These are passed to relevant compiler provided they are supported
 set(GCC_AND_CLANG_WARNINGS
-    "-Wall"
+  "-Wall"
 )
 set(GCC_ONLY_WARNINGS "")
 set(CLANG_ONLY_WARNINGS "")
 set(MSVC_WARNINGS "/W3")
 
+################################################################################
+# Serious warnings
+################################################################################
+# This declares the flags that are passed to the compiler when
+# `WARNINGS_AS_ERRORS` is set to `SERIOUS_ONLY`. Only flags that are supported
+# by the compiler are used.
+#
+# In effect this a "whitelist" approach where we explicitly tell the compiler
+# which warnings we want to be treated as errors. The alternative would be a
+# "blacklist" approach where we ask the compiler to treat all warnings are
+# treated as errors but then we explicitly list which warnings which should be
+# allowed.
+#
+# The "whitelist" approach seems simpiler because we can incrementally add
+# warnings we "think are serious".
+
+# TODO: Add more warnings that are considered serious enough that we should
+# treat them as errors.
+set(GCC_AND_CLANG_WARNINGS_AS_ERRORS
+  # https://clang.llvm.org/docs/DiagnosticsReference.html#wodr
+  "-Werror=odr"
+)
+set(GCC_WARNINGS_AS_ERRORS
+  ""
+)
+set(CLANG_WARNINGS_AS_ERRORS
+  # https://clang.llvm.org/docs/DiagnosticsReference.html#wdelete-non-virtual-dtor
+  "-Werror=delete-non-virtual-dtor"
+  # https://clang.llvm.org/docs/DiagnosticsReference.html#woverloaded-virtual
+  "-Werror=overloaded-virtual"
+)
+
+################################################################################
+# Test warning/error flags
+################################################################################
 set(WARNING_FLAGS_TO_CHECK "")
+set(WARNING_AS_ERROR_FLAGS_TO_CHECK "")
 if ("${CMAKE_CXX_COMPILER_ID}" MATCHES "GNU")
   list(APPEND WARNING_FLAGS_TO_CHECK ${GCC_AND_CLANG_WARNINGS})
   list(APPEND WARNING_FLAGS_TO_CHECK ${GCC_ONLY_WARNINGS})
+  list(APPEND WARNING_AS_ERROR_FLAGS_TO_CHECK ${GCC_AND_CLANG_WARNINGS_AS_ERRORS})
+  list(APPEND WARNING_AS_ERROR_FLAGS_TO_CHECK ${GCC_WARNINGS_AS_ERRORS})
 elseif ("${CMAKE_CXX_COMPILER_ID}" MATCHES "Clang")
   list(APPEND WARNING_FLAGS_TO_CHECK ${GCC_AND_CLANG_WARNINGS})
   list(APPEND WARNING_FLAGS_TO_CHECK ${CLANG_ONLY_WARNINGS})
+  list(APPEND WARNING_AS_ERROR_FLAGS_TO_CHECK ${GCC_AND_CLANG_WARNINGS_AS_ERRORS})
+  list(APPEND WARNING_AS_ERROR_FLAGS_TO_CHECK ${CLANG_WARNINGS_AS_ERRORS})
   # FIXME: Remove "x.." when CMP0054 is set to NEW
 elseif ("x${CMAKE_CXX_COMPILER_ID}" STREQUAL "xMSVC")
   list(APPEND WARNING_FLAGS_TO_CHECK ${MSVC_WARNINGS})
@@ -31,8 +75,40 @@ foreach (flag ${WARNING_FLAGS_TO_CHECK})
   z3_add_cxx_flag("${flag}")
 endforeach()
 
-option(WARNINGS_AS_ERRORS "Treat compiler warnings as errors" OFF)
-if (WARNINGS_AS_ERRORS)
+# TODO: Remove this eventually.
+# Detect legacy `WARNINGS_AS_ERRORS` boolean option and covert to new
+# to new option type.
+get_property(
+  WARNINGS_AS_ERRORS_CACHE_VAR_TYPE
+  CACHE
+  WARNINGS_AS_ERRORS
+  PROPERTY
+  TYPE
+)
+if ("${WARNINGS_AS_ERRORS_CACHE_VAR_TYPE}" STREQUAL "BOOL")
+  message(WARNING "Detected legacy WARNINGS_AS_ERRORS option. Upgrading")
+  set(WARNINGS_AS_ERRORS_DEFAULT "${WARNINGS_AS_ERRORS}")
+  # Delete old entry
+  unset(WARNINGS_AS_ERRORS CACHE)
+else()
+  set(WARNINGS_AS_ERRORS_DEFAULT "SERIOUS_ONLY")
+endif()
+
+set(WARNINGS_AS_ERRORS
+  ${WARNINGS_AS_ERRORS_DEFAULT}
+  CACHE STRING
+  "Treat warnings as errors. ON, OFF, or SERIOUS_ONLY"
+)
+ # Set GUI options
+set_property(
+  CACHE
+  WARNINGS_AS_ERRORS
+  PROPERTY STRINGS
+  "ON;OFF;SERIOUS_ONLY"
+)
+
+if ("${WARNINGS_AS_ERRORS}" STREQUAL "ON")
+  message(STATUS "Treating compiler warnings as errors")
   if (("${CMAKE_CXX_COMPILER_ID}" MATCHES "Clang") OR ("${CMAKE_CXX_COMPILER_ID}" MATCHES "GNU"))
     list(APPEND Z3_COMPONENT_CXX_FLAGS "-Werror")
     # FIXME: Remove "x.." when CMP0054 is set to NEW
@@ -41,8 +117,14 @@ if (WARNINGS_AS_ERRORS)
   else()
     message(AUTHOR_WARNING "Unknown compiler")
   endif()
-  message(STATUS "Treating compiler warnings as errors")
-else()
+elseif ("${WARNINGS_AS_ERRORS}" STREQUAL "SERIOUS_ONLY")
+  message(STATUS "Treating only serious compiler warnings as errors")
+  # Loop through the flags
+  foreach (flag ${WARNING_AS_ERROR_FLAGS_TO_CHECK})
+    # Add globally because some flags need to be passed at link time.
+    z3_add_cxx_flag("${flag}" GLOBAL)
+  endforeach()
+elseif ("${WARNINGS_AS_ERRORS}" STREQUAL "OFF")
   message(STATUS "Not treating compiler warnings as errors")
   # FIXME: Remove "x.." when CMP0054 is set to NEW
   if ("x${CMAKE_CXX_COMPILER_ID}" STREQUAL "xMSVC")
@@ -51,4 +133,8 @@ else()
     # build system.
     list(APPEND Z3_COMPONENT_CXX_FLAGS "/WX-")
   endif()
+else()
+  message(FATAL_ERROR
+    "WARNINGS_AS_ERRORS set to unsupported value \"${WARNINGS_AS_ERRORS}\""
+  )
 endif()

cmake/z3_add_cxx_flag.cmake

@@ -2,7 +2,7 @@ include(CheckCXXCompilerFlag)
 include(CMakeParseArguments)
 
 function(z3_add_cxx_flag flag)
-  CMAKE_PARSE_ARGUMENTS(z3_add_flag "REQUIRED" "" "" ${ARGN})
+  CMAKE_PARSE_ARGUMENTS(z3_add_flag "REQUIRED;GLOBAL" "" "" ${ARGN})
   string(REPLACE "-" "_" SANITIZED_FLAG_NAME "${flag}")
   string(REPLACE "/" "_" SANITIZED_FLAG_NAME "${SANITIZED_FLAG_NAME}")
   string(REPLACE "=" "_" SANITIZED_FLAG_NAME "${SANITIZED_FLAG_NAME}")
@@ -16,8 +16,13 @@ function(z3_add_cxx_flag flag)
   endif()
   if (HAS_${SANITIZED_FLAG_NAME})
     message(STATUS "C++ compiler supports ${flag}")
-    list(APPEND Z3_COMPONENT_CXX_FLAGS "${flag}")
-    set(Z3_COMPONENT_CXX_FLAGS "${Z3_COMPONENT_CXX_FLAGS}" PARENT_SCOPE)
+    if (z3_add_flag_GLOBAL)
+      # Set globally
+      set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${flag} " PARENT_SCOPE)
+    else()
+      list(APPEND Z3_COMPONENT_CXX_FLAGS "${flag}")
+      set(Z3_COMPONENT_CXX_FLAGS "${Z3_COMPONENT_CXX_FLAGS}" PARENT_SCOPE)
+    endif()
   else()
     message(STATUS "C++ compiler does not support ${flag}")
   endif()