From 25699ac284c59b88148da9d35987c52cb84376d0 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 | 3 +++ CMakeLists.txt | 8 +++++++- 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 8c7f6f854f0..d637e99d508 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: diff --git a/CMakeLists.txt b/CMakeLists.txt index 8703ad3dd3d..0f856af9fd7 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -6,13 +6,19 @@ 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(NOT MSVC AND 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 4b10f7e9f49..cf147c01a6c 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 c4fbcce41e6..961c6e53e07 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 29bf78e2d57..e4204d39bad 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());