Fix Clang build, add Clang CI, warnings-as-errors

Author: vector-of-bool

.github/workflows/ci.yml

@@ -18,12 +18,26 @@ jobs:
         run: make format-check PYTHON_RUN="uv run --group=format"
 
   build-alpine:
-    name: Build on Alpine 3.20
+    strategy:
+      matrix:
+        alpine_version:
+          - 3.21
+          - 3.20
+          - 3.19
+        # This arg is formatted in this way so that it is easily understandable in
+        # the GitHub Actions web interface
+        vcpkg_arg: &vcpkg_arg
+          - use_vcpkg=false
+          - use_vcpkg=true
+    name: Build on Alpine
     runs-on: ubuntu-latest
     steps:
       - *checkout
       - name: Run Build
-        run: bash tools/earthly.sh +build-alpine --use_vcpkg=false
+        run: |
+          bash tools/earthly.sh +build-alpine \
+            --alpine_version=${{matrix.alpine_version}} \
+            --${{matrix.vcpkg_arg}}
 
   build-gcc-matrix:
     strategy:
@@ -34,12 +48,29 @@ jobs:
           - 14.3.0
           - 13.4.0
           - 12.5.0
+
     runs-on: ubuntu-latest
     name: "Build with GCC"
     steps:
       - *checkout
       - name: Build
         run: |
           bash tools/earthly.sh +build-gcc \
-            --gcc_version=${{matrix.gcc_version}} \
-            --test=false
+            --gcc_version=${{matrix.gcc_version}}
+
+  build-clang-matrix:
+    strategy:
+      fail-fast: true
+      matrix:
+        version_arg:
+          - clang_major_version=20
+          - clang_major_version=19
+        # vcpkg_arg: *vcpkg_arg
+    runs-on: ubuntu-latest
+    name: Build with Clang
+    steps:
+      - *checkout
+      - name: Build
+        run: |
+          bash tools/earthly.sh +build-clang \
+            --${{matrix.version_arg}}

CMakeLists.txt

@@ -15,6 +15,11 @@ if(AMONGOC_USE_PMM)
     pmm(VCPKG REVISION 2024.08.23)
 endif()
 
+# A toggle to enable warnings-as-errors. We don't use CMAKE_COMPILE_WARNING_AS_ERROR,
+# because we don't want to enable -Werror on dependencies that we might be importing
+# and building
+option(AMONGOC_COMPILE_WARNING_AS_ERROR "Treat compiler warnings as errors" FALSE)
+
 # Set up `find_package()` to trigger FetchContent for certain deps
 include(DeclareFetchContentDeps)
 
@@ -160,7 +165,11 @@ target_link_libraries(amongoc
         $<BUILD_INTERFACE:asio::asio>
         $<BUILD_INTERFACE:neo::fun>
         OpenSSL::SSL
-    )
+)
+
+# Toggle warnings-as-errors
+set_property(TARGET amongoc PROPERTY COMPILE_WARNING_AS_ERROR "${AMONGOC_COMPILE_WARNING_AS_ERROR}")
+
 if(BUILD_TESTING)
     # Add Catch integrations
     include(Catch)
@@ -173,15 +182,20 @@ if(BUILD_TESTING)
         $<BUILD_INTERFACE:asio::asio>
         $<BUILD_INTERFACE:neo::fun>
         Boost::container
-        )
+    )
     target_include_directories(amongoc-test PRIVATE src)
-    catch_discover_tests(amongoc-test DISCOVERY_MODE PRE_TEST
+    catch_discover_tests(
+        amongoc-test
+        DISCOVERY_MODE PRE_TEST
         PROPERTIES
             SKIP_REGULAR_EXPRESSION ":[0-9]+: SKIPPED:"
             TIMEOUT 5
-        )
+    )
     target_compile_features(amongoc-test PRIVATE cxx_std_23)
 
+    # Also add -Werror to the test target, if requested for amongoc
+    set_property(TARGET amongoc-test PROPERTY COMPILE_WARNING_AS_ERROR "${AMONGOC_COMPILE_WARNING_AS_ERROR}")
+
     # A test case that simply invokes each BSON API
     add_executable(bson-use-test tests/use-bson.test.c)
     target_link_libraries(bson-use-test PRIVATE amongoc::amongoc)

Earthfile

@@ -1,16 +1,37 @@
 VERSION 0.8
 
+# Tweak the default container registry used for pulling system images.
 ARG --global default_container_registry = "docker.io"
 
 build-gcc:
     ARG --required gcc_version
     FROM $default_container_registry/gcc:$gcc_version
+    ARG warnings_as_errors=true
     DO --pass-args +BOOTSTRAP_BUILD_INSTALL_EXPORT \
-        --build_deps "perl pkg-config linux-libc-dev curl zip unzip ccache" \
-        --third_deps "libfmt-dev libboost-url1.81-dev libboost-container1.81-dev libssl-dev"
+        --warnings_as_errors=$warnings_as_errors \
+        --build_deps "ccache" \
+        --vcpkg_bs_deps "build-essential perl git pkg-config linux-libc-dev curl zip unzip"
+
+build-clang:
+    ARG --required clang_version_major
+    FROM $default_container_registry/ubuntu:24.04
+    DO +INIT
+    # Required for the LLVM installer:
+    RUN __install lsb-release software-properties-common gnupg
+    RUN curl -Ls https://apt.llvm.org/llvm.sh -o llvm.sh && \
+        bash llvm.sh "$clang_version_major"
+    ENV CC=clang-$clang_version_major
+    ENV CXX=clang++-$clang_version_major
+    ARG warnings_as_errors=true
+    DO --pass-args +BOOTSTRAP_BUILD_INSTALL_EXPORT \
+        --warnings_as_errors=$warnings_as_errors \
+        --build_deps "ccache" \
+        --vcpkg_bs_deps "build-essential perl git pkg-config linux-libc-dev curl zip unzip"
 
 build-alpine:
-    FROM $default_container_registry/alpine:3.20
+    ARG alpine_version=3.20
+    FROM $default_container_registry/alpine:$alpine_version
+    ARG warnings_as_errors=true
     DO --pass-args +BOOTSTRAP_BUILD_INSTALL_EXPORT \
         --build_deps "build-base git cmake gcc g++ ninja make ccache python3" \
         --vcpkg_bs_deps "pkgconfig linux-headers perl bash tar zip unzip curl" \
@@ -19,8 +40,6 @@ build-alpine:
 build-debian:
     FROM $default_container_registry/debian:12
     DO --pass-args +BOOTSTRAP_BUILD_INSTALL_EXPORT \
-        # Spec test generation requires a Python newer than what is on Debian 12
-        --BUILD_SPEC_TESTS=FALSE \
         --build_deps "build-essential cmake git ninja-build python3 ccache" \
         --vcpkg_bs_deps "perl pkg-config linux-libc-dev curl zip unzip" \
         --third_deps "libfmt-dev libboost-url1.81-dev libboost-container1.81-dev libssl-dev"
@@ -69,14 +88,16 @@ run:
 # Miscellaneous system init
 INIT:
     FUNCTION
-    COPY --chmod=755 tools/__install /usr/local/bin/__install
+    COPY --chmod=755 tools/__install tools/__bool tools/__boolstr /usr/local/bin/
     RUN __install curl
     ARG uv_version = "0.8.15"
     ARG uv_install_sh_url = "https://astral.sh/uv/$uv_version/install.sh"
-    RUN (curl -LsSf "$uv_install_sh_url" || wget -qO- "$uv_install_sh_url") \
-        | env UV_UNMANAGED_INSTALL=/opt/uv sh - \
-        && ln -s /opt/uv/uv /usr/local/bin/uv \
-        && uv --version
+    IF ! uv --version
+        RUN (curl -LsSf "$uv_install_sh_url" || wget -qO- "$uv_install_sh_url") \
+            | env UV_UNMANAGED_INSTALL=/opt/uv sh - \
+            && ln -s /opt/uv/uv /usr/local/bin/uv \
+            && uv --version
+    END
 
 BOOTSTRAP_BUILD_INSTALL_EXPORT:
     FUNCTION
@@ -97,10 +118,10 @@ BOOTSTRAP_DEPS:
     RUN __install $build_deps
     # Switch behavior based on whether we use vcpkg
     ARG use_vcpkg=true
-    IF ! $use_vcpkg
+    IF ! __bool $use_vcpkg
         # No vcpkg. Install system dependencies
-        ARG third_deps
-        RUN __install $third_deps
+        COPY tools/ci/install-third-deps.sh /
+        RUN bash /install-third-deps.sh
         # Install system deps for testing, if needed
         ARG test_deps
         ARG test=true
@@ -126,7 +147,7 @@ BOOTSTRAP_DEPS:
         # Running CMake now will prepare our dependencies without configuring the rest of the project
         CACHE ~/.cache/vcpkg
         ARG launcher
-        RUN $launcher uv run --with=cmake~=3.20 --with=ninja cmake -S $src_tmp -B $src_tmp/_build/vcpkg-bootstrapping
+        RUN $launcher uv run --with=cmake~=3.20 --with=ninja cmake -G Ninja -S $src_tmp -B $src_tmp/_build/vcpkg-bootstrapping
     END
 
 COPY_SRC:
@@ -144,19 +165,21 @@ BUILD:
     CACHE ~/.cache/ccache
     # Toggle testing
     ARG test=true
-    LET __test=$(echo $test | tr [:lower:] [:upper:])
-    ARG BUILD_SPEC_TESTS=TRUE
+    # Enable -Werror
+    ARG warnings_as_errors=false
     # Toggle PMM in the build
     ARG use_vcpkg=true
-    LET __use_vcpkg=$(echo "$use_vcpkg" | tr "[:lower:]" "[:upper:]")
+    # The configurations to build (semicolon-separated list)
+    ARG configs=Debug
     # Configure
     RUN $launcher make test \
             LAUNCHER='uv run --group=build' \
-            CONFIGS="Debug" \
+            CONFIGS="$configs" \
             TEST_CONFIG="Debug" \
-            USE_PMM=$__use_vcpkg \
             INSTALL_PREFIX=$install_prefix \
-            BUILD_TESTING=$__test
+            USE_PMM=$(__boolstr $use_vcpkg) \
+            WARNINGS_AS_ERRORS=$(__boolstr $warnings_as_errors) \
+            BUILD_TESTING=$(__boolstr $test)
     IF test "$install_prefix" != ""
         FOR conf IN Debug # Release RelWithDebInfo
             RUN $launcher make install-fast LAUNCHER='uv run --group=build' INSTALL_PREFIX=$install_prefix INSTALL_CONFIG=$conf

Makefile

@@ -1,8 +1,8 @@
 # *** Build Parameters ***
 # Whether to use PMM for the build process
-USE_PMM := TRUE
+USE_PMM := true
 # Whether to build tests
-BUILD_TESTING := TRUE
+BUILD_TESTING := true
 # Sanitizers to request (Comma-separated list)
 SANITIZE :=
 # The configurations to build (Semicolon-separated, or "all")
@@ -11,6 +11,8 @@ CONFIGS := Debug
 TEST_CONFIG := Debug
 # Set the CMAKE_INSTALL_PREFIX and the `--prefix` arg for installs
 INSTALL_PREFIX :=
+# Treat compiler warnings as errors (Sets COMPILE_WARNING_AS_ERROR on amongoc)
+WARNINGS_AS_ERRORS := false
 
 # *** Execution Parameters ***
 # Set the LAUNCHER parameter to prefix all executed commands
@@ -55,6 +57,7 @@ configure:
 		-B "$(BUILD_DIR)" \
 		-D CMAKE_CROSS_CONFIGS="$(CONFIGS)" \
 		-D CMAKE_DEFAULT_CONFIGS=all \
+		-D AMONGOC_COMPILE_WARNING_AS_ERROR=$(WARNINGS_AS_ERRORS) \
 		-D AMONGOC_USE_PMM=$(USE_PMM) \
 		-D BUILD_TESTING=$(BUILD_TESTING) \
 		-D MONGO_SANITIZE="$(SANITIZE)" \
@@ -92,10 +95,10 @@ package-fast:
 	rm -r -- "$(CPACK_OUT)/_CPack_Packages"
 
 format-check:
-	$(PYTHON_RUN) tools/format.py --mode=check
+	uv run --group=format --isolated $(PYTHON_RUN) tools/format.py --mode=check
 
 format:
-	$(PYTHON_RUN) tools/format.py
+	uv run --group=format --isolated $(PYTHON_RUN) tools/format.py
 
 packages:
 	bash $(THIS_DIR)/tools/earthly.sh -a +build-multi/ _build/pkgs

include/bson/doc.h

@@ -402,11 +402,11 @@ class document {
 
     using enable_trivially_relocatable = document;
 
-#if !mlib_audit_allocator_passing()
+#if mlib_allocator_default_constructible()
     document() noexcept
-        : document(allocator_type(mlib_default_allocator)) {}
+        : document(allocator_type()) {}
     explicit document(bson_view v)
-        : document(v, allocator_type(mlib_default_allocator)) {}
+        : document(v, allocator_type()) {}
 #endif
 
     ~document() { _del(); }

include/mlib/alloc.h

@@ -108,6 +108,10 @@ mlib_extern_c_end();
 
 #if mlib_is_cxx()
 
+#ifndef mlib_allocator_default_constructible
+#define mlib_allocator_default_constructible() !mlib_audit_allocator_passing()
+#endif
+
 namespace mlib {
 
 /**
@@ -121,6 +125,11 @@ class allocator {
     using value_type = T;
     using pointer    = value_type*;
 
+#if mlib_allocator_default_constructible()
+    allocator() noexcept
+        : _alloc(::mlib_default_allocator) {}
+#endif
+
     // Construct around an existing mlib_allocator object
     constexpr allocator(mlib_allocator a) noexcept
         : _alloc(a) {}

include/mlib/config.h

@@ -191,7 +191,7 @@
  * @brief Macro that should be used to toggle convenience APIs that will
  * pass default allocators.
  */
-#define mlib_audit_allocator_passing() 1
+#define mlib_audit_allocator_passing() 0
 #endif  // mlib_audit_allocator_passing
 
 #ifdef __GNUC__
@@ -263,6 +263,10 @@
     MLIB_IF_GCC(mlib_pragma(GCC diagnostic ignored Warning))                                       \
     mlib_static_assert(true, "")
 
+#define mlib_clang_warning_disable(Warning)                                                        \
+    MLIB_IF_CLANG(mlib_pragma(clang diagnostic ignored Warning))                                   \
+    mlib_static_assert(true, "")
+
 #define mlib_gnu_warning_disable(Warning)                                                          \
     MLIB_IF_GNU_LIKE(mlib_pragma(GCC diagnostic ignored Warning))                                  \
     mlib_static_assert(true, "")
@@ -342,7 +346,7 @@
 #define MLIB_ARGC_PICK(Prefix, ...)                                                                \
     MLIB_PASTE_3(Prefix, _argc_, MLIB_ARG_COUNT(__VA_ARGS__))(__VA_ARGS__)
 
-MLIB_LANG_PICK()([[noreturn]]) mlib_constexpr void mlib_unreachable() mlib_noexcept {
+MLIB_LANG_PICK()([[noreturn]]) static inline void mlib_unreachable() mlib_noexcept {
     MLIB_IF_GNU_LIKE(__builtin_unreachable();)
     MLIB_IF_MSVC(__assume(0);)
 }

src/amongoc/status.test.cpp

@@ -10,7 +10,7 @@ TEST_CASE("Status/Okay") {
     CHECK(st.category == &amongoc_generic_category);
 
     bool took_else = false;
-    amongoc_if_error (st, _) {
+    amongoc_if_error (st, _ [[maybe_unused]]) {
         FAIL_CHECK("Did not expect an error");
     } else {
         took_else = true;
@@ -23,7 +23,7 @@ TEST_CASE("Status/From an Error") {
     CHECK(st.category == &amongoc_generic_category);
     CHECK(st.code == EIO);
     bool took_err = false;
-    amongoc_if_error (st, _) {
+    amongoc_if_error (st, _ [[maybe_unused]]) {
         took_err = true;
     } else {
         FAIL_CHECK("Did not take the error branch");

src/amongoc/uri.cpp

@@ -9,7 +9,9 @@
 #include <mlib/alloc.h>
 
 #include <boost/url.hpp>
-#include <fmt/chrono.h>  // Enable duration formatting
+// XXX: We would like to format date types, but a Clang bug prevents it from
+//      working. Disable that for now.
+// #include <fmt/chrono.h>  // Enable duration formatting
 #include <neo/tokenize.hpp>
 #include <neo/utility.hpp>
 
@@ -201,14 +203,21 @@ result<connection_uri> connection_uri::parse(std::string_view
             return [=](I ival) -> I {
                 if (ival < min or ival > max) {
                     warn.fire(defer_convert([&] {
-                        return uri_warning_event(
-                            format(alloc,
-                                   "URI parameter “{}”: Value {} is outside the supported range "
-                                   "(min: {}, max: {})",
-                                   std::string_view(qp.key),
-                                   ival,
-                                   min,
-                                   max));
+                        if constexpr (fmt::is_formattable<I>::value) {
+                            return uri_warning_event(format(
+                                alloc,
+                                "URI parameter “{}”: Value {} is outside the supported range "
+                                "(min: {}, max: {})",
+                                std::string_view(qp.key),
+                                ival,
+                                min,
+                                max));
+                        } else {
+                            return uri_warning_event(
+                                format(alloc,
+                                       "URI parameter “{}” is outside the supported range",
+                                       std::string_view(qp.key)));
+                        }
                     }));
                 }
                 return std::clamp(ival, min, max);