Skip to content

Commit 902bfc7

Browse files
SiarheiFedartsouSiarheiFedartsou
authored
Refactor CMake code related to compiler warnings, enable some additional warnings (#6355)
1 parent 3c5d99b commit 902bfc7

23 files changed

+142
-93
lines changed

‎.github/workflows/osrm-backend.yml

Lines changed: 1 addition & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -335,7 +335,6 @@ jobs:
335335
BUILD_TYPE: Release
336336
CCOMPILER: clang-6.0
337337
CXXCOMPILER: clang++-6.0
338-
ENABLE_GLIBC_WORKAROUND: ON
339338
ENABLE_CONAN: ON
340339
NODE_PACKAGE_TESTS_ONLY: ON
341340

@@ -347,7 +346,6 @@ jobs:
347346
BUILD_TYPE: Debug
348347
CCOMPILER: clang-6.0
349348
CXXCOMPILER: clang++-6.0
350-
ENABLE_GLIBC_WORKAROUND: ON
351349
ENABLE_CONAN: ON
352350
NODE_PACKAGE_TESTS_ONLY: ON
353351

@@ -359,7 +357,6 @@ jobs:
359357
BUILD_TYPE: Release
360358
CCOMPILER: clang-6.0
361359
CXXCOMPILER: clang++-6.0
362-
ENABLE_GLIBC_WORKAROUND: ON
363360
ENABLE_CONAN: ON
364361
NODE_PACKAGE_TESTS_ONLY: ON
365362

@@ -371,7 +368,6 @@ jobs:
371368
BUILD_TYPE: Debug
372369
CCOMPILER: clang-6.0
373370
CXXCOMPILER: clang++-6.0
374-
ENABLE_GLIBC_WORKAROUND: ON
375371
ENABLE_CONAN: ON
376372
NODE_PACKAGE_TESTS_ONLY: ON
377373

@@ -384,7 +380,6 @@ jobs:
384380
BUILD_TYPE: Release
385381
CCOMPILER: clang-6.0
386382
CXXCOMPILER: clang++-6.0
387-
ENABLE_GLIBC_WORKAROUND: ON
388383
ENABLE_CONAN: ON
389384
NODE_PACKAGE_TESTS_ONLY: ON
390385

@@ -396,7 +391,6 @@ jobs:
396391
BUILD_TYPE: Debug
397392
CCOMPILER: clang-6.0
398393
CXXCOMPILER: clang++-6.0
399-
ENABLE_GLIBC_WORKAROUND: ON
400394
ENABLE_CONAN: ON
401395
NODE_PACKAGE_TESTS_ONLY: ON
402396

@@ -433,7 +427,6 @@ jobs:
433427
BUILD_TYPE: Release
434428
CCOMPILER: clang-6.0
435429
CXXCOMPILER: clang++-6.0
436-
ENABLE_GLIBC_WORKAROUND: ON
437430
ENABLE_CONAN: ON
438431
NODE_PACKAGE_TESTS_ONLY: ON
439432

@@ -445,7 +438,6 @@ jobs:
445438
BUILD_TYPE: Debug
446439
CCOMPILER: clang-6.0
447440
CXXCOMPILER: clang++-6.0
448-
ENABLE_GLIBC_WORKAROUND: ON
449441
ENABLE_CONAN: ON
450442
NODE_PACKAGE_TESTS_ONLY: ON
451443

@@ -482,7 +474,6 @@ jobs:
482474
BUILD_TYPE: Release
483475
CCOMPILER: clang-6.0
484476
CXXCOMPILER: clang++-6.0
485-
ENABLE_GLIBC_WORKAROUND: ON
486477
ENABLE_CONAN: ON
487478
NODE_PACKAGE_TESTS_ONLY: ON
488479

@@ -494,7 +485,6 @@ jobs:
494485
BUILD_TYPE: Debug
495486
CCOMPILER: clang-6.0
496487
CXXCOMPILER: clang++-6.0
497-
ENABLE_GLIBC_WORKAROUND: ON
498488
ENABLE_CONAN: ON
499489
NODE_PACKAGE_TESTS_ONLY: ON
500490

@@ -513,7 +503,6 @@ jobs:
513503
ENABLE_ASSERTIONS: ${{ matrix.ENABLE_ASSERTIONS }}
514504
ENABLE_CLANG_TIDY: ${{ matrix.ENABLE_CLANG_TIDY }}
515505
ENABLE_COVERAGE: ${{ matrix.ENABLE_COVERAGE }}
516-
ENABLE_GLIBC_WORKAROUND: ${{ matrix.ENABLE_GLIBC_WORKAROUND }}
517506
ENABLE_CONAN: ${{ matrix.ENABLE_CONAN }}
518507
ENABLE_SANITIZER: ${{ matrix.ENABLE_SANITIZER }}
519508
NODE_PACKAGE_TESTS_ONLY: ${{ matrix.NODE_PACKAGE_TESTS_ONLY }}
@@ -671,8 +660,8 @@ jobs:
671660
-DBUILD_TOOLS=${BUILD_TOOLS:-OFF} \
672661
-DENABLE_CCACHE=ON \
673662
-DCMAKE_INSTALL_PREFIX=${OSRM_INSTALL_DIR} \
674-
-DENABLE_GLIBC_WORKAROUND=${ENABLE_GLIBC_WORKAROUND:-OFF} \
675663
"${APPLE_SILICON_FLAGS[@]}"
664+
676665
make --jobs=${JOBS}
677666
678667
if [[ "${NODE_PACKAGE_TESTS_ONLY}" != "ON" && "${ENABLE_APPLE_SILICON}" != "ON" ]]; then

‎CMakeLists.txt

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,6 @@ option(ENABLE_LTO "Use LTO if available" OFF)
3535
option(ENABLE_FUZZING "Fuzz testing using LLVM's libFuzzer" OFF)
3636
option(ENABLE_GOLD_LINKER "Use GNU gold linker if available" ON)
3737
option(ENABLE_NODE_BINDINGS "Build NodeJs bindings" OFF)
38-
option(ENABLE_GLIBC_WORKAROUND "Workaround GLIBC symbol exports" OFF)
3938
option(ENABLE_CLANG_TIDY "Enables clang-tidy checks" OFF)
4039

4140
if (ENABLE_CLANG_TIDY)
@@ -337,17 +336,17 @@ if (ENABLE_SANITIZER)
337336
endif()
338337

339338
# Configuring compilers
340-
set(OSRM_WARNING_FLAGS "-Werror=all -Werror=extra -Werror=uninitialized -Werror=unreachable-code -Werror=unused-variable -Werror=unreachable-code -Wno-error=cpp -Wpedantic")
339+
include(cmake/warnings.cmake)
341340
if(CMAKE_CXX_COMPILER_ID MATCHES "Clang")
342-
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${OSRM_WARNING_FLAGS} -Werror=strict-overflow=2 -Wno-error=unused-local-typedef -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fPIC -fcolor-diagnostics -ftemplate-depth=1024 -Wno-unused-command-line-argument")
341+
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fPIC -fcolor-diagnostics -ftemplate-depth=1024")
343342
elseif(CMAKE_CXX_COMPILER_ID MATCHES "GNU")
344343
set(COLOR_FLAG "-fdiagnostics-color=auto")
345344
check_cxx_compiler_flag("-fdiagnostics-color=auto" HAS_COLOR_FLAG)
346345
if(NOT HAS_COLOR_FLAG)
347346
set(COLOR_FLAG "")
348347
endif()
349348
# using GCC
350-
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${OSRM_WARNING_FLAGS} -Werror=strict-overflow=1 -Wno-error=maybe-uninitialized -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 ${COLOR_FLAG} -fPIC -ftemplate-depth=1024")
349+
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 ${COLOR_FLAG} -fPIC -ftemplate-depth=1024")
351350

352351
if(WIN32) # using mingw
353352
add_dependency_defines(-DWIN32)
@@ -440,6 +439,8 @@ include_directories(SYSTEM ${CHEAPRULER_INCLUDE_DIR})
440439

441440
add_library(MICROTAR OBJECT "${CMAKE_CURRENT_SOURCE_DIR}/third_party/microtar/src/microtar.c")
442441
set_property(TARGET MICROTAR PROPERTY POSITION_INDEPENDENT_CODE ON)
442+
target_no_warning(MICROTAR unused-variable)
443+
target_no_warning(MICROTAR format)
443444

444445
set(PROTOZERO_INCLUDE_DIR "${CMAKE_CURRENT_SOURCE_DIR}/third_party/protozero/include")
445446
include_directories(SYSTEM ${PROTOZERO_INCLUDE_DIR})
@@ -861,10 +862,6 @@ add_custom_target(uninstall
861862
add_subdirectory(unit_tests)
862863
add_subdirectory(src/benchmarks)
863864

864-
if (ENABLE_GLIBC_WORKAROUND)
865-
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -DGLIBC_WORKAROUND")
866-
endif()
867-
868865
if (ENABLE_NODE_BINDINGS)
869866
add_subdirectory(src/nodejs)
870867
endif()

‎cmake/warnings.cmake

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
1+
include (CheckCXXCompilerFlag)
2+
include (CheckCCompilerFlag)
3+
4+
# Try to add -Wflag if compiler supports it
5+
macro (add_warning flag)
6+
string(REPLACE "-" "_" underscored_flag ${flag})
7+
string(REPLACE "+" "x" underscored_flag ${underscored_flag})
8+
9+
check_cxx_compiler_flag("-W${flag}" SUPPORTS_CXXFLAG_${underscored_flag})
10+
check_c_compiler_flag("-W${flag}" SUPPORTS_CFLAG_${underscored_flag})
11+
12+
if (SUPPORTS_CXXFLAG_${underscored_flag})
13+
set (CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -W${flag}")
14+
else()
15+
message (STATUS "Flag -W${flag} is unsupported")
16+
endif()
17+
18+
if (SUPPORTS_CFLAG_${underscored_flag})
19+
set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -W${flag}")
20+
else()
21+
message(STATUS "Flag -W${flag} is unsupported")
22+
endif()
23+
endmacro()
24+
25+
# Try to add -Wno flag if compiler supports it
26+
macro (no_warning flag)
27+
add_warning(no-${flag})
28+
endmacro ()
29+
30+
31+
# The same but only for specified target.
32+
macro (target_add_warning target flag)
33+
string (REPLACE "-" "_" underscored_flag ${flag})
34+
string (REPLACE "+" "x" underscored_flag ${underscored_flag})
35+
36+
check_cxx_compiler_flag("-W${flag}" SUPPORTS_CXXFLAG_${underscored_flag})
37+
38+
if (SUPPORTS_CXXFLAG_${underscored_flag})
39+
target_compile_options (${target} PRIVATE "-W${flag}")
40+
else ()
41+
message (STATUS "Flag -W${flag} is unsupported")
42+
endif ()
43+
endmacro ()
44+
45+
macro (target_no_warning target flag)
46+
target_add_warning(${target} no-${flag})
47+
endmacro ()
48+
49+
add_warning(all)
50+
add_warning(extra)
51+
add_warning(pedantic)
52+
add_warning(error) # treat all warnings as errors
53+
if(CMAKE_CXX_COMPILER_ID MATCHES "Clang")
54+
add_warning(strict-overflow=2)
55+
elseif(CMAKE_CXX_COMPILER_ID MATCHES "GNU")
56+
add_warning(strict-overflow=1)
57+
endif()
58+
add_warning(suggest-override)
59+
add_warning(suggest-destructor-override)
60+
add_warning(unused)
61+
add_warning(unreachable-code)
62+
add_warning(delete-incomplete)
63+
add_warning(duplicated-cond)
64+
add_warning(disabled-optimization)
65+
add_warning(init-self)
66+
add_warning(bool-compare)
67+
add_warning(logical-not-parentheses)
68+
add_warning(logical-op)
69+
add_warning(maybe-uninitialized)
70+
add_warning(misleading-indentation)
71+
# `no-` prefix is part of warning name(i.e. doesn't mean we are disabling it)
72+
add_warning(no-return-local-addr)
73+
add_warning(odr)
74+
add_warning(pointer-arith)
75+
add_warning(redundant-decls)
76+
add_warning(reorder)
77+
add_warning(shift-negative-value)
78+
add_warning(sizeof-array-argument)
79+
add_warning(switch-bool)
80+
add_warning(tautological-compare)
81+
add_warning(trampolines)
82+
no_warning(c++17-extensions)
83+
# TODO: these warnings are not enabled by default, but we consider them as useful and good to enable in the future
84+
no_warning(implicit-int-conversion)
85+
no_warning(implicit-float-conversion)
86+
no_warning(unused-member-function)
87+
no_warning(old-style-cast)
88+
no_warning(non-virtual-dtor)
89+
no_warning(float-conversion)
90+
no_warning(sign-conversion)
91+
no_warning(shorten-64-to-32)
92+
no_warning(padded)
93+
no_warning(missing-noreturn)

‎include/contractor/contract_excludable_graph.hpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ inline auto contractExcludableGraph(ContractorGraph contractor_graph_,
5656
// By not contracting all contractable nodes we avoid creating
5757
// a very dense core. This increases the overall graph sizes a little bit
5858
// but increases the final CH quality and contraction speed.
59-
constexpr float BASE_CORE = 0.9;
59+
constexpr float BASE_CORE = 0.9f;
6060
is_shared_core =
6161
contractGraph(contractor_graph, std::move(always_allowed), node_weights, BASE_CORE);
6262

‎include/engine/api/base_api.hpp

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -123,9 +123,9 @@ class BaseAPI
123123

124124
const auto &snapped_location = candidatesSnappedLocation(candidates);
125125
const auto &input_location = candidatesInputLocation(candidates);
126-
auto location =
127-
fbresult::Position(static_cast<double>(util::toFloating(snapped_location.lon)),
128-
static_cast<double>(util::toFloating(snapped_location.lat)));
126+
auto location = fbresult::Position(
127+
static_cast<float>(static_cast<double>(util::toFloating(snapped_location.lon))),
128+
static_cast<float>(static_cast<double>(util::toFloating(snapped_location.lat))));
129129

130130
const auto toName = [this](const auto &phantom) {
131131
return facade.GetNameForID(facade.GetNameIndex(phantom.forward_segment_id.id))
@@ -156,8 +156,8 @@ class BaseAPI
156156

157157
auto waypoint = std::make_unique<fbresult::WaypointBuilder>(*builder);
158158
waypoint->add_location(&location);
159-
waypoint->add_distance(
160-
util::coordinate_calculation::greatCircleDistance(snapped_location, input_location));
159+
waypoint->add_distance(static_cast<float>(
160+
util::coordinate_calculation::greatCircleDistance(snapped_location, input_location)));
161161
waypoint->add_name(name_string);
162162
if (parameters.generate_hints)
163163
{

‎include/engine/engine.hpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ template <typename Algorithm> class Engine final : public EngineInterface
8383

8484
Engine(const Engine &) = delete;
8585
Engine &operator=(const Engine &) = delete;
86-
virtual ~Engine() = default;
86+
virtual ~Engine() override = default;
8787

8888
Status Route(const api::RouteParameters &params, api::ResultT &result) const override final
8989
{

‎include/engine/routing_algorithms.hpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ template <typename Algorithm> class RoutingAlgorithms final : public RoutingAlgo
7272
{
7373
}
7474

75-
virtual ~RoutingAlgorithms() = default;
75+
virtual ~RoutingAlgorithms() override = default;
7676

7777
InternalManyRoutesResult
7878
AlternativePathSearch(const PhantomEndpointCandidates &endpoint_candidates,

‎include/storage/shared_monitor.hpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ template <typename Data> struct SharedMonitor
7575

7676
region = bi::mapped_region(shmem, bi::read_write);
7777
}
78-
catch (const bi::interprocess_exception &exception)
78+
catch (const bi::interprocess_exception &)
7979
{
8080
auto message = boost::format("No shared memory block '%1%' found, have you forgotten "
8181
"to run osrm-datastore?") %
@@ -124,7 +124,7 @@ template <typename Data> struct SharedMonitor
124124
bi::shared_memory_object shmem_open =
125125
bi::shared_memory_object(bi::open_only, Data::name, bi::read_only);
126126
}
127-
catch (const bi::interprocess_exception &exception)
127+
catch (const bi::interprocess_exception &)
128128
{
129129
return false;
130130
}

‎include/util/exception.hpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,7 @@ class RuntimeError : public exception
106106
// This function exists to 'anchor' the class, and stop the compiler from
107107
// copying vtable and RTTI info into every object file that includes
108108
// this header. (Caught by -Wweak-vtables under Clang.)
109-
virtual void anchor() const;
109+
virtual void anchor() const override;
110110
const ErrorCode code;
111111

112112
static std::string BuildMessage(const std::string &message,

‎include/util/filtered_integer_range.hpp

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -87,11 +87,11 @@ template <typename Integer, typename Filter> class filtered_range
8787

8888
// convenience function to construct an integer range with type deduction
8989
template <typename Integer, typename Filter>
90-
filtered_range<Integer, Filter>
91-
filtered_irange(const Integer first,
92-
const Integer last,
93-
const Filter &filter,
94-
typename std::enable_if<std::is_integral<Integer>::value>::type * = 0) noexcept
90+
filtered_range<Integer, Filter> filtered_irange(
91+
const Integer first,
92+
const Integer last,
93+
const Filter &filter,
94+
typename std::enable_if<std::is_integral<Integer>::value>::type * = nullptr) noexcept
9595
{
9696
return filtered_range<Integer, Filter>(first, last, filter);
9797
}

0 commit comments

Comments
 (0)