From 6b35668bb3f3a2a74ceafb51e64cc21b28467235 Mon Sep 17 00:00:00 2001 From: Keenan Gugeler Date: Thu, 2 Nov 2023 11:24:00 -0400 Subject: [PATCH] build: enable Werror on non-Windows Because of our changes recently, we can now build the project with -Werror. However, this doesn't enable all warnings. We should add warnings to the list of flagged warnings so as to forbid more and more bad code. Windows is simply too noisy to enable -Werror for right now. --- .github/workflows/ci-workflow.yml | 4 ++++ CMakeLists.txt | 7 ++++++- Makefile | 27 ++++++++++++++++----------- src/common/types/int128_t.cpp | 4 ++-- tools/shell/embedded_shell.cpp | 8 ++------ 5 files changed, 30 insertions(+), 20 deletions(-) diff --git a/.github/workflows/ci-workflow.yml b/.github/workflows/ci-workflow.yml index 8c7f6f854f..16ece9d6a4 100644 --- a/.github/workflows/ci-workflow.yml +++ b/.github/workflows/ci-workflow.yml @@ -1,5 +1,8 @@ name: CI +env: + WERROR: 1 + on: pull_request: branches: @@ -180,6 +183,7 @@ jobs: CARGO_BUILD_JOBS: 18 NUM_THREADS: 18 TEST_JOBS: 9 + WERROR: 0 steps: - uses: actions/checkout@v3 diff --git a/CMakeLists.txt b/CMakeLists.txt index 19133e1c54..cd1d35234f 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -6,13 +6,18 @@ find_package(Threads REQUIRED) set(CMAKE_FIND_PACKAGE_RESOLVE_SYMLINKS TRUE) set(CMAKE_CXX_STANDARD 20) -set(CMAKE_CXX_STANDARD_REQUIRED True) +set(CMAKE_CXX_STANDARD_REQUIRED TRUE) set(CMAKE_POSITION_INDEPENDENT_CODE ON) if(NOT APPLE) set(CMAKE_CXX_VISIBILITY_PRESET hidden) set(CMAKE_VISIBILITY_INLINES_HIDDEN ON) endif() +option(ENABLE_WERROR "Treat all warnings as errors" FALSE) +if(ENABLE_WERROR) + set(CMAKE_COMPILE_WARNING_AS_ERROR TRUE) +endif() + # Detect OS and architecture, copied from DuckDB set(OS_NAME "unknown") set(OS_ARCH "amd64") diff --git a/Makefile b/Makefile index e12e4f3446..9b061ace22 100644 --- a/Makefile +++ b/Makefile @@ -5,6 +5,7 @@ FORCE_COLOR= NUM_THREADS ?= 1 TEST_JOBS ?= 10 SANITIZER_FLAG= +WERROR_FLAG= ROOT_DIR=$(CURDIR) export CMAKE_BUILD_PARALLEL_LEVEL=$(NUM_THREADS) @@ -30,6 +31,10 @@ ifeq ($(UBSAN), 1) SANITIZER_FLAG=-DENABLE_ADDRESS_SANITIZER=FALSE -DENABLE_THREAD_SANITIZER=TRUE -DENABLE_UBSAN=TRUE endif +ifeq ($(WERROR), 1) + WERROR_FLAG=-DENABLE_WERROR=TRUE +endif + ifeq ($(OS),Windows_NT) define mkdirp (if not exist "$(1)" mkdir "$(1)") @@ -42,61 +47,61 @@ endif release: $(call mkdirp,build/release) && cd build/release && \ - cmake $(GENERATOR) $(FORCE_COLOR) $(SANITIZER_FLAG) -DCMAKE_BUILD_TYPE=Release ../.. && \ + cmake $(GENERATOR) $(FORCE_COLOR) $(SANITIZER_FLAG) $(WERROR_FLAG) -DCMAKE_BUILD_TYPE=Release ../.. && \ cmake --build . --config Release debug: $(call mkdirp,build/debug) && cd build/debug && \ - cmake $(GENERATOR) $(FORCE_COLOR) $(SANITIZER_FLAG) -DCMAKE_BUILD_TYPE=Debug ../.. && \ + cmake $(GENERATOR) $(FORCE_COLOR) $(SANITIZER_FLAG) $(WERROR_FLAG) -DCMAKE_BUILD_TYPE=Debug ../.. && \ cmake --build . --config Debug all: $(call mkdirp,build/release) && cd build/release && \ - cmake $(GENERATOR) $(FORCE_COLOR) $(SANITIZER_FLAG) -DCMAKE_BUILD_TYPE=Release -DBUILD_TESTS=TRUE -DBUILD_BENCHMARK=TRUE -DBUILD_NODEJS=TRUE -DBUILD_EXAMPLES=TRUE ../.. && \ + cmake $(GENERATOR) $(FORCE_COLOR) $(SANITIZER_FLAG) $(WERROR_FLAG) -DCMAKE_BUILD_TYPE=Release -DBUILD_TESTS=TRUE -DBUILD_BENCHMARK=TRUE -DBUILD_NODEJS=TRUE -DBUILD_EXAMPLES=TRUE ../.. && \ cmake --build . --config Release example: $(call mkdirp,build/release) && cd build/release && \ - cmake $(GENERATOR) $(FORCE_COLOR) $(SANITIZER_FLAG) -DCMAKE_BUILD_TYPE=Release -DBUILD_EXAMPLES=TRUE ../.. && \ + cmake $(GENERATOR) $(FORCE_COLOR) $(SANITIZER_FLAG) $(WERROR_FLAG) -DCMAKE_BUILD_TYPE=Release -DBUILD_EXAMPLES=TRUE ../.. && \ cmake --build . --config Release alldebug: $(call mkdirp,build/debug) && cd build/debug && \ - cmake $(GENERATOR) $(FORCE_COLOR) $(SANITIZER_FLAG) -DCMAKE_BUILD_TYPE=Debug -DBUILD_TESTS=TRUE -DBUILD_BENCHMARK=TRUE -DBUILD_NODEJS=TRUE -DBUILD_EXAMPLES=TRUE ../.. && \ + cmake $(GENERATOR) $(FORCE_COLOR) $(SANITIZER_FLAG) $(WERROR_FLAG) -DCMAKE_BUILD_TYPE=Debug -DBUILD_TESTS=TRUE -DBUILD_BENCHMARK=TRUE -DBUILD_NODEJS=TRUE -DBUILD_EXAMPLES=TRUE ../.. && \ cmake --build . --config Debug benchmark: $(call mkdirp,build/release) && cd build/release && \ - cmake $(GENERATOR) $(FORCE_COLOR) $(SANITIZER_FLAG) -DCMAKE_BUILD_TYPE=Release -DBUILD_BENCHMARK=TRUE ../.. && \ + cmake $(GENERATOR) $(FORCE_COLOR) $(SANITIZER_FLAG) $(WERROR_FLAG) -DCMAKE_BUILD_TYPE=Release -DBUILD_BENCHMARK=TRUE ../.. && \ cmake --build . --config Release nodejs: $(call mkdirp,build/release) && cd build/release && \ - cmake $(GENERATOR) $(FORCE_COLOR) $(SANITIZER_FLAG) -DCMAKE_BUILD_TYPE=Release -DBUILD_NODEJS=TRUE ../.. && \ + cmake $(GENERATOR) $(FORCE_COLOR) $(SANITIZER_FLAG) $(WERROR_FLAG) -DCMAKE_BUILD_TYPE=Release -DBUILD_NODEJS=TRUE ../.. && \ cmake --build . --config Release java: $(call mkdirp,build/release) && cd build/release && \ - cmake $(GENERATOR) $(FORCE_COLOR) $(SANITIZER_FLAG) -DCMAKE_BUILD_TYPE=Release -DBUILD_JAVA=TRUE ../.. && \ + cmake $(GENERATOR) $(FORCE_COLOR) $(SANITIZER_FLAG) $(WERROR_FLAG) -DCMAKE_BUILD_TYPE=Release -DBUILD_JAVA=TRUE ../.. && \ cmake --build . --config Release test: $(call mkdirp,build/release) && cd build/release && \ - cmake $(GENERATOR) $(FORCE_COLOR) $(SANITIZER_FLAG) -DCMAKE_BUILD_TYPE=Release -DBUILD_TESTS=TRUE ../.. && \ + cmake $(GENERATOR) $(FORCE_COLOR) $(SANITIZER_FLAG) $(WERROR_FLAG) -DCMAKE_BUILD_TYPE=Release -DBUILD_TESTS=TRUE ../.. && \ cmake --build . --config Release cd $(ROOT_DIR)/build/release/test && \ ctest --output-on-failure -j ${TEST_JOBS} lcov: $(call mkdirp,build/release) && cd build/release && \ - cmake $(GENERATOR) $(FORCE_COLOR) $(SANITIZER_FLAG) -DCMAKE_BUILD_TYPE=Release -DBUILD_TESTS=TRUE -DBUILD_NODEJS=TRUE -DBUILD_LCOV=TRUE ../.. && \ + cmake $(GENERATOR) $(FORCE_COLOR) $(SANITIZER_FLAG) $(WERROR_FLAG) -DCMAKE_BUILD_TYPE=Release -DBUILD_TESTS=TRUE -DBUILD_NODEJS=TRUE -DBUILD_LCOV=TRUE ../.. && \ cmake --build . --config Release cd $(ROOT_DIR)/build/release/test && \ ctest --output-on-failure -j ${TEST_JOBS} clangd: $(call mkdirp,build/release) && cd build/release && \ - cmake $(GENERATOR) $(FORCE_COLOR) $(SANITIZER_FLAG) -DCMAKE_EXPORT_COMPILE_COMMANDS=1 ../.. + cmake $(GENERATOR) $(FORCE_COLOR) $(SANITIZER_FLAG) $(WERROR_FLAG) -DCMAKE_EXPORT_COMPILE_COMMANDS=1 ../.. tidy: clangd run-clang-tidy -p build/release -quiet -j $(NUM_THREADS) \ diff --git a/src/common/types/int128_t.cpp b/src/common/types/int128_t.cpp index c4fbcce41e..961c6e53e0 100644 --- a/src/common/types/int128_t.cpp +++ b/src/common/types/int128_t.cpp @@ -427,8 +427,8 @@ bool CastInt128ToFloating(int128_t input, REAL_T& result) { result = -REAL_T(function::NumericLimits::maximum() - input.low) - 1; break; default: - result = - REAL_T(input.high) * function::NumericLimits::maximum() + REAL_T(input.low); + result = REAL_T(input.high) * REAL_T(function::NumericLimits::maximum()) + + REAL_T(input.low); break; } return true; diff --git a/tools/shell/embedded_shell.cpp b/tools/shell/embedded_shell.cpp index 29bf78e2d5..e4204d39ba 100644 --- a/tools/shell/embedded_shell.cpp +++ b/tools/shell/embedded_shell.cpp @@ -6,11 +6,7 @@ #include #include "catalog/catalog.h" -#include "common/logging_level_utils.h" #include "common/string_utils.h" -#include "common/type_utils.h" -#include "json.hpp" -#include "processor/result/factorized_table.h" #include "utf8proc.h" #include "utf8proc_wrapper.h" @@ -260,7 +256,7 @@ void EmbeddedShell::run() { } } -void EmbeddedShell::interruptHandler(int /*signal*/) { +void EmbeddedShell::interruptHandler(int /*signal*/) { globalConnection->interrupt(); } @@ -352,7 +348,7 @@ void EmbeddedShell::printExecutionResult(QueryResult& queryResult) const { if (numTuples == 1) { printf("(1 tuple)\n"); } else { - printf("(%lu tuples)\n", numTuples); + printf("(%" PRIu64 " tuples)\n", numTuples); } printf("Time: %.2fms (compiling), %.2fms (executing)\n", querySummary->getCompilingTime(), querySummary->getExecutionTime());