Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Aggressive compilation warnings #2304

Merged
merged 1 commit into from
Feb 20, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 4 additions & 6 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -23,22 +23,20 @@ option(BUILD_LIBMAMBAPY "Build libmamba Python bindings" OFF)
option(BUILD_LIBMAMBA_TESTS "Build libmamba C++ tests" OFF)
option(BUILD_MICROMAMBA "Build micromamba" OFF)
option(BUILD_MAMBA_PACKAGE "Build mamba package utility" OFF)
option(MAMBA_WARNING_AS_ERROR "Treat compiler warnings as errors" OFF)

include("cmake/CompilerWarnings.cmake")

if (MSVC)
# NOMINMAX : prevent tons of code to be included when having to `#include <windows.h>`
# /EHsc : enable C++ exceptions (otherwise exceptions do not work)
# /Zc:__cplusplus : makes sure `__cplusplus` is set to the current C++ version language. Otherwise it is always set to an incorrect wrong value.
# /MP : enable multi-process build with MSBuild (it should be on by default but CMake generates the project files without the right params).
# /W4 : warnings level 4 (default in visual studio projects and recommended minimum level).
# /external:I $ENV{CONDA_PREFIX}: consider the conda env prefix libraries headers as "external" to this project.
# /external:W0 : set the warning level to 1 for external libraries headers (severe warnings, default when unspecified) to only see important warnings coming from dependencies headers.
string(REGEX REPLACE "/W[0-9]" "/W4" CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS}")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /D_CRT_SECURE_NO_WARNINGS /DNOMINMAX /EHsc /Zc:__cplusplus /MP /experimental:external /external:I $ENV{CONDA_PREFIX} /external:W1")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did you remove this on purpose?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I moved the warnings to the CompilerWarnings.cmake file. Other options are left here.

set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /D_CRT_SECURE_NO_WARNINGS /DNOMINMAX /EHsc /Zc:__cplusplus /MP /experimental:external /external:I $ENV{CONDA_PREFIX}")
# Force release mode to avoid debug libraries to be linked
set(CMAKE_BUILD_TYPE Release)
# add_definitions("-DUNICODE -D_UNICODE")
else()
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall")
endif()

# Variants
Expand Down
144 changes: 144 additions & 0 deletions cmake/CompilerWarnings.cmake
Original file line number Diff line number Diff line change
@@ -0,0 +1,144 @@
# Module to set default compiler warnings.
#
# File adapted from Jason Turner's cpp_starter_project
# https://github.com/lefticus/cpp_starter_project/blob/master/cmake/CompilerWarnings.cmake Using
# INTERFACE targets is not so desirable as they need to be installed when building static libraries.

function(mamba_target_add_compile_warnings target)
# Names of option parameters (without arguments)
set(options)
# Names of named parameters with a single argument
set(oneValueArgs WARNING_AS_ERROR)
# Names of named parameters with a multiple arguments
set(multiValueArgs)
cmake_parse_arguments(ARG "${options}" "${oneValueArgs}" "${multiValueArgs}" ${ARGN})

if(ARG_UNPARSED_ARGUMENTS)
message(
AUTHOR_WARNING
"Unrecoginzed options passed to ${CMAKE_CURRENT_FUNCTION}: "
"${ARG_UNPARSED_ARGUMENTS}"
)
endif()

set(
msvc_warnings
# External sever warnings
/experimental:external /external:W1
# Baseline reasonable warnings
/W4
# "identfier": conversion from "type1" to "type1", possible loss of data
/w14242
# "operator": conversion from "type1:field_bits" to "type2:field_bits", possible loss of
# data
/w14254
# "function": member function does not override any base class virtual member function
/w14263
# "classname": class has virtual functions, but destructor is not virtual instances of this
# class may not be destructed correctly
/w14265
# "operator": unsigned/negative constant mismatch
/w14287
# Nonstandard extension used: "variable": loop control variable declared in the for-loop is
# used outside the for-loop scope
/we4289
# "operator": expression is always "boolean_value"
/w14296
# "variable": pointer truncation from "type1" to "type2"
/w14311
# Expression before comma evaluates to a function which is missing an argument list
/w14545
# Function call before comma missing argument list
/w14546
# "operator": operator before comma has no effect; expected operator with side-effect
/w14547
# "operator": operator before comma has no effect; did you intend "operator"?
/w14549
# Expression has no effect; expected expression with side- effect
/w14555
# Pragma warning: there is no warning number "number"
/w14619
# Enable warning on thread un-safe static member initialization
/w14640
# Conversion from "type1" to "type_2" is sign-extended. This may cause unexpected runtime
# behavior.
/w14826
# Wide string literal cast to "LPSTR"
/w14905
# String literal cast to "LPWSTR"
/w14906
# Illegal copy-initialization; more than one user-defined conversion has been implicitly
# applied
/w14928
)

set(
clang_warnings
# Some default set of warnings
-Wall
# Reasonable and standard
-Wextra
# Warn the user if a variable declaration shadows one from a parent context
-Wshadow
# Warn the user if a class with virtual functions has a non-virtual destructor. This helps
# catch hard to track down memory errors
-Wnon-virtual-dtor
# Warn for c-style casts
-Wold-style-cast
# Warn for potential performance problem casts
-Wcast-align
# Warn on anything being unused
-Wunused
# Warn if you overload (not override) a virtual function
-Woverloaded-virtual
# Warn if non-standard C++ is used
-Wpedantic
# Warn on type conversions that may lose data
-Wconversion
# Warn on sign conversions
-Wsign-conversion
# Warn if a null dereference is detected
-Wnull-dereference
# Warn if float is implicit promoted to double
-Wdouble-promotion
# Warn on security issues around functions that format output (ie printf)
-Wformat=2
# Warn on code that cannot be executed
-Wunreachable-code
# Warn if a variable is used before being initialized
-Wuninitialized
)

if(${ARG_WARNING_AS_ERROR})
set(clang_warnings ${clang_warnings} -Werror)
set(msvc_warnings ${msvc_warnings} /WX)
endif()

set(
gcc_warnings
${clang_warnings}
# Warn if identation implies blocks where blocks do not exist
-Wmisleading-indentation
# Warn if if / else chain has duplicated conditions
-Wduplicated-cond
# Warn if if / else branches have duplicated code
-Wduplicated-branches
# Warn about logical operations being used where bitwise were probably wanted
-Wlogical-op
# Warn if you perform a cast to the same type
-Wuseless-cast
)

if(MSVC)
set(warnings ${msvc_warnings})
elseif(CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang")
set(warnings ${clang_warnings})
elseif(CMAKE_CXX_COMPILER_ID STREQUAL "Clang")
set(warnings ${clang_warnings})
else()
set(warnings ${gcc_warnings})
endif()

target_compile_options("${target}" PRIVATE ${warnings})

endfunction()
5 changes: 5 additions & 0 deletions libmamba/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,11 @@ macro(libmamba_create_target target_name linkage deps_linkage output_name)
# ======
add_library(${target_name} ${linkage_upper} ${LIBMAMBA_SOURCES} ${LIBMAMBA_HEADERS})

mamba_target_add_compile_warnings(
${target_name}
WARNING_AS_ERROR ${MAMBA_WARNING_AS_ERROR}
)

if (${deps_linkage_upper} STREQUAL "STATIC")
message(" -> Statically linking against libmamba (static) dependencies")
if (UNIX)
Expand Down
1 change: 1 addition & 0 deletions libmamba/tests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ set(LIBMAMBA_TEST_SRCS
message(STATUS "Building libmamba C++ tests")

add_executable(test_libmamba ${LIBMAMBA_TEST_SRCS})
mamba_target_add_compile_warnings(test_libmamba WARNING_AS_ERROR ${MAMBA_WARNING_AS_ERROR})

target_include_directories(
test_libmamba
Expand Down
2 changes: 2 additions & 0 deletions libmambapy/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ pybind11_add_module(bindings
longpath.manifest
)

mamba_target_add_compile_warnings(bindings WARNING_AS_ERROR ${MAMBA_WARNING_AS_ERROR})

target_link_libraries(bindings PUBLIC pybind11::pybind11 libmamba)
set_property(TARGET bindings PROPERTY CXX_STANDARD 17)

Expand Down
1 change: 1 addition & 0 deletions mamba_package/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ endif ()
# ================

add_executable(mamba-package ${MAMBA_PACKAGE_SRCS} ${MAMBA_PACKAGE_HEADERS})
mamba_target_add_compile_warnings(mamba-package WARNING_AS_ERROR ${MAMBA_WARNING_AS_ERROR})

if (${MAMBA_PACKAGE_LINKAGE} STREQUAL "FULL_STATIC")
target_link_libraries(mamba-package PRIVATE libmamba-full-static)
Expand Down
2 changes: 2 additions & 0 deletions micromamba/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@ set(MICROMAMBA_HEADERS

add_executable(micromamba ${MICROMAMBA_SRCS} ${MICROMAMBA_HEADERS})

mamba_target_add_compile_warnings(micromamba WARNING_AS_ERROR ${MAMBA_WARNING_AS_ERROR})

if(NOT (TARGET libmamba OR TARGET libmamba-static OR TARGET libmamba-full-static))
find_package(libmamba REQUIRED)
endif ()
Expand Down