Address review feedback: make -Werror opt-in, centralize warning flags

- Gate -Werror, -Wundef, -Wnull-dereference, -Wmissing-prototypes behind
  ROARING_WERROR option (OFF by default) so downstream builds are unaffected
- Centralize warning flag variables (C_EXTRA_WARNING_FLAGS,
  AMALG_EXTRA_WARNING_FLAGS) in FindOptions.cmake
- Simplify preprocessor guards: use #if defined(X) instead of
  #if defined(X) && X for macros guaranteed non-zero when defined
- Add defined(__STDC_VERSION__) guard for -Wundef clean C++ compilation
- Restore containers.c comments removed in previous commit

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Author: maxwbuckley

CMakeLists.txt

@@ -45,6 +45,7 @@ option(ROARING_LINK_STATIC "Link executables (tests, benchmarks) statically" OFF
 option(ROARING_BUILD_LTO "Build library with Link Time Optimization" OFF)
 option(ROARING_BUILD_C_AS_CPP "Build library C files using C++ compilation" OFF)
 option(ROARING_BUILD_C_TESTS_AS_CPP "Build test C files using C++ compilation" OFF)
+option(ROARING_WERROR "Treat warnings as errors (developer only)" OFF)
 option(ROARING_SANITIZE "Sanitize addresses" OFF)
 option(ROARING_SANITIZE_THREADS "Sanitize threads" OFF)
 option(ROARING_SANITIZE_UNDEFINED "Sanitize undefined behaviors" OFF)
@@ -146,21 +147,16 @@ if(ENABLE_ROARING_TESTS AND BASH AND NOT EMSCRIPTEN)
 
   add_executable(amalgamate_demo $<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}/amalgamation_demo.c>)
   target_link_libraries(amalgamate_demo croaring-singleheader-include-source)
-  # The amalgamation compiles everything in a single translation unit, which
-  # gives GCC deeper interprocedural visibility and triggers false-positive
-  # null-dereference warnings through allocation-failure paths. Suppress
-  # this warning for amalgamation targets only; the library itself is still
-  # checked with -Wnull-dereference in separate compilation units.
-  if(NOT MSVC)
-    target_compile_options(amalgamate_demo PRIVATE -Wno-null-dereference)
+  if(AMALG_EXTRA_WARNING_FLAGS)
+    target_compile_options(amalgamate_demo PRIVATE ${AMALG_EXTRA_WARNING_FLAGS})
   endif()
   add_test(amalgamate_demo amalgamate_demo)
 
   add_library(croaring-singleheader-source-lib $<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}/roaring.c>)
   set_target_properties(croaring-singleheader-source-lib PROPERTIES WINDOWS_EXPORT_ALL_SYMBOLS YES)
   target_include_directories(croaring-singleheader-source-lib PUBLIC $<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}>)
-  if(NOT MSVC)
-    target_compile_options(croaring-singleheader-source-lib PRIVATE -Wno-null-dereference)
+  if(AMALG_EXTRA_WARNING_FLAGS)
+    target_compile_options(croaring-singleheader-source-lib PRIVATE ${AMALG_EXTRA_WARNING_FLAGS})
   endif()
 
   add_executable(amalgamate_demo_cpp $<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}/amalgamation_demo.cpp>)

include/roaring/misc/configreport.h

@@ -186,14 +186,14 @@ static inline void tellmeall() {
                (long unsigned int)sizeof(size_t),
                (long unsigned int)sizeof(int));
     }
-#if defined(__LITTLE_ENDIAN__) && __LITTLE_ENDIAN__
+#if defined(__LITTLE_ENDIAN__)
 // This is what we expect!
 // printf("you have little endian machine");
 #endif
-#if defined(__BIG_ENDIAN__) && __BIG_ENDIAN__
+#if defined(__BIG_ENDIAN__)
     printf("you have a big endian machine");
 #endif
-#if defined(__CHAR_BIT__) && __CHAR_BIT__
+#if defined(__CHAR_BIT__)
     if (__CHAR_BIT__ != 8) printf("on your machine, chars don't have 8bits???");
 #endif
     if (computecacheline() != 64)
@@ -217,14 +217,14 @@ static inline void tellmeall() {
                (long unsigned int)sizeof(size_t),
                (long unsigned int)sizeof(int));
     }
-#if defined(__LITTLE_ENDIAN__) && __LITTLE_ENDIAN__
+#if defined(__LITTLE_ENDIAN__)
 // This is what we expect!
 // printf("you have little endian machine");
 #endif
-#if defined(__BIG_ENDIAN__) && __BIG_ENDIAN__
+#if defined(__BIG_ENDIAN__)
     printf("you have a big endian machine");
 #endif
-#if defined(__CHAR_BIT__) && __CHAR_BIT__
+#if defined(__CHAR_BIT__)
     if (__CHAR_BIT__ != 8) printf("on your machine, chars don't have 8bits???");
 #endif
 }

include/roaring/portability.h

@@ -471,7 +471,7 @@ static inline int roaring_hamming(uint64_t x) {
    // We lack __has_include to check:
 #define CROARING_ATOMIC_IMPL CROARING_ATOMIC_IMPL_CPP
 #endif  //__has_include
-#elif __STDC_VERSION__ >= 201112L && !defined(__STDC_NO_ATOMICS__)
+#elif defined(__STDC_VERSION__) && __STDC_VERSION__ >= 201112L && !defined(__STDC_NO_ATOMICS__)
 #define CROARING_ATOMIC_IMPL CROARING_ATOMIC_IMPL_C
 #elif CROARING_REGULAR_VISUAL_STUDIO
    // https://www.technetworkhub.com/c11-atomics-in-visual-studio-2022-version-17/
@@ -584,7 +584,7 @@ static inline uint32_t croaring_refcount_get(const croaring_refcount_t *val) {
 
 // We want to initialize structs to zero portably (C and C++), without
 // warnings. We can do mystruct s = CROARING_ZERO_INITIALIZER;
-#if defined(__cplusplus) && __cplusplus
+#if defined(__cplusplus)
 #define CROARING_ZERO_INITIALIZER \
     {}
 #else

src/CMakeLists.txt

@@ -63,8 +63,8 @@ target_link_libraries(roaring PUBLIC "$<BUILD_INTERFACE:roaring-headers>")
 target_link_libraries(roaring PUBLIC "$<BUILD_INTERFACE:roaring-headers-cpp>")
 
 target_include_directories(roaring PRIVATE ${CMAKE_CURRENT_SOURCE_DIR})
-if(NOT MSVC)
-  target_compile_options(roaring PRIVATE $<$<COMPILE_LANGUAGE:C>:-Wmissing-prototypes>)
+if(C_EXTRA_WARNING_FLAGS)
+  target_compile_options(roaring PRIVATE $<$<COMPILE_LANGUAGE:C>:${C_EXTRA_WARNING_FLAGS}>)
 endif()
 
 #

src/containers/containers.c

@@ -52,7 +52,8 @@ extern bool container_iterator_prev(const container_t *c, uint8_t typecode,
                                     uint16_t *value);
 extern bool container_contains(
     const container_t *c, uint16_t val,
-    uint8_t typecode);
+    uint8_t typecode  // !!! should be second argument?
+);
 
 void container_free(container_t *c, uint8_t type) {
     switch (type) {
@@ -162,7 +163,7 @@ extern inline container_t *container_add(container_t *c, uint16_t val,
                                          uint8_t *new_typecode);
 
 extern inline bool container_contains(const container_t *c, uint16_t val,
-                                      uint8_t typecode);
+                                      uint8_t typecode);  // !!! 2nd arg?
 
 extern inline container_t *container_and(const container_t *c1, uint8_t type1,
                                          const container_t *c2, uint8_t type2,

tests/cpp_unit.cpp

@@ -1476,7 +1476,7 @@ DEFINE_TEST(test_cpp_frozen_64) {
     roaring_aligned_free(buf);
 }
 
-#if defined(ROARING_UNSAFE_FROZEN_TESTS) && ROARING_UNSAFE_FROZEN_TESTS
+#if defined(ROARING_UNSAFE_FROZEN_TESTS)
 // This test is unsafe, as it may trigger unaligned memory access
 // It is only enabled if ROARING_UNSAFE_FROZEN_TESTS is defined.
 DEFINE_TEST(test_cpp_frozen_portable) {
@@ -1553,7 +1553,7 @@ DEFINE_TEST(test_cpp_frozen_portable) {
 }
 #endif  // ROARING_UNSAFE_FROZEN_TESTS
 
-#if defined(ROARING_UNSAFE_FROZEN_TESTS) && ROARING_UNSAFE_FROZEN_TESTS
+#if defined(ROARING_UNSAFE_FROZEN_TESTS)
 // This test is unsafe, as it may trigger unaligned memory access
 // It is only enabled if ROARING_UNSAFE_FROZEN_TESTS is defined.
 DEFINE_TEST(test_cpp_frozen_64_portable) {
@@ -2215,7 +2215,7 @@ int main() {
         cmocka_unit_test(test_cpp_bidirectional_iterator_64),
         cmocka_unit_test(test_cpp_frozen),
         cmocka_unit_test(test_cpp_frozen_64),
-#if defined(ROARING_UNSAFE_FROZEN_TESTS) && ROARING_UNSAFE_FROZEN_TESTS
+#if defined(ROARING_UNSAFE_FROZEN_TESTS)
         cmocka_unit_test(test_cpp_frozen_portable),
         cmocka_unit_test(test_cpp_frozen_64_portable),
 #endif  // ROARING_UNSAFE_FROZEN_TESTS

tests/toplevel_unit.c

@@ -4745,7 +4745,7 @@ DEFINE_TEST(test_frozen_serialization_max_containers) {
     frozen_serialization_compare(r);
 }
 
-#if defined(ROARING_UNSAFE_FROZEN_TESTS) && ROARING_UNSAFE_FROZEN_TESTS
+#if defined(ROARING_UNSAFE_FROZEN_TESTS)
 // This test is unsafe, as it may trigger unaligned memory access
 // It is only enabled if ROARING_UNSAFE_FROZEN_TESTS is defined.
 DEFINE_TEST(test_portable_deserialize_frozen) {
@@ -5177,7 +5177,7 @@ int main() {
 #if !CROARING_IS_BIG_ENDIAN
         cmocka_unit_test(test_frozen_serialization),
         cmocka_unit_test(test_frozen_serialization_max_containers),
-#if defined(ROARING_UNSAFE_FROZEN_TESTS) && ROARING_UNSAFE_FROZEN_TESTS
+#if defined(ROARING_UNSAFE_FROZEN_TESTS)
         cmocka_unit_test(test_portable_deserialize_frozen),
 #endif  // ROARING_UNSAFE_FROZEN_TESTS
         cmocka_unit_test(issue_15jan2024),

tools/cmake/FindOptions.cmake

@@ -24,12 +24,20 @@ endif()
 endif()
 
 if(NOT MSVC)
-set(WARNING_FLAGS "-Wall -Werror -Wnull-dereference -Wundef")
+set(WARNING_FLAGS "-Wall")
 if (CMAKE_CXX_COMPILER_ID STREQUAL "Clang")
 set(WARNING_FLAGS "${WARNING_FLAGS} -Wmissing-braces -Wextra -Wsign-compare -Wshadow -Wwrite-strings -Wpointer-arith -Winit-self")
 elseif (CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
 set(WARNING_FLAGS "${WARNING_FLAGS} -Wextra -Wsign-compare -Wshadow -Wwrite-strings -Wpointer-arith -Winit-self -Wcast-align")
 endif()
+if(ROARING_WERROR)
+  set(WARNING_FLAGS "${WARNING_FLAGS} -Werror -Wundef -Wnull-dereference")
+  set(C_EXTRA_WARNING_FLAGS "-Wmissing-prototypes")
+  # The amalgamation compiles everything in a single translation unit, which
+  # gives GCC deeper interprocedural visibility and triggers false-positive
+  # null-dereference warnings through allocation-failure paths.
+  set(AMALG_EXTRA_WARNING_FLAGS -Wno-null-dereference)
+endif()
 endif()
 
 set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} ${STD_FLAGS} ${OPT_FLAGS} ${INCLUDE_FLAGS} ${WARNING_FLAGS} ")