C++ Warning fixes, werror on ci, C++17 compliance (#2957)

### What * Fixes #2903 * Part of #2919 * currate warning list for gcc/clang * enable warnings as errors on CI, for simplicity it's always active on roundtrip tests * enable standard set of warning settings on all C++ targets * fix warnings in codegen'ed code and manually written code * fix accidental C++20 (should have been caught in theory by warnings but it didn't trigger for unknown reason) ### Checklist * [x] I have read and agree to [Contributor Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and the [Code of Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md) * [x] I've included a screenshot or gif (if applicable) * [x] I have tested [demo.rerun.io](https://demo.rerun.io/pr/2957) (if applicable) - [PR Build Summary](https://build.rerun.io/pr/2957) - [Docs preview](https://rerun.io/preview/pr%3Aandreas%2Fcpp%2Fwarning-fixes-and-ci-werror/docs) - [Examples preview](https://rerun.io/preview/pr%3Aandreas%2Fcpp%2Fwarning-fixes-and-ci-werror/examples)
rerun-io · Aug 15, 2023 · 16635be · 16635be
1 parent 04f974f
commit 16635be
Show file tree

Hide file tree

Showing 73 changed files with 541 additions and 364 deletions.
diff --git a/.github/workflows/reusable_checks.yml b/.github/workflows/reusable_checks.yml
@@ -340,12 +340,12 @@ jobs:
 
       - name: Build and run C++ minimal example
         shell: bash
-        run: ./examples/cpp/minimal/build_and_run.sh
+        run: ./examples/cpp/minimal/build_and_run.sh --werror
 
       - name: Build and run rerun_cpp tests
         shell: bash
-        run: ./rerun_cpp/build_and_run_tests.sh
+        run: ./rerun_cpp/build_and_run_tests.sh --werror
 
       - name: Build code examples
         shell: bash
-        run: ./tests/cpp/build_all_doc_examples.sh
+        run: ./tests/cpp/build_all_doc_examples.sh --werror
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -6,14 +6,43 @@ function(set_default_warning_settings target)
     if(MSVC)
         target_compile_options(${target} PRIVATE /W4 /WX)
     else()
-        target_compile_options(${target} PRIVATE -Wall -Wextra -Wpedantic -Wcast-align -Wcast-qual -Wformat=2 -Wmissing-include-dirs -Wnull-dereference -Woverloaded-virtual -Wpointer-arith -Wshadow -Wswitch-enum -Wvla -Wno-sign-compare -Wconversion -Wunused -Wold-style-cast -Wno-missing-braces)
+        # Enabled warnings.
+        target_compile_options(${target} PRIVATE
+            -Wall
+            -Wcast-align
+            -Wcast-qual
+            -Wextra
+            -Wformat=2
+            -Wmissing-include-dirs
+            -Wnull-dereference
+            -Wold-style-cast
+            -Wpedantic
+            -Wpointer-arith
+            -Wshadow
+            -Wswitch-enum
+            -Wvla
+        )
 
+        if(CMAKE_CXX_COMPILER_ID MATCHES "Clang") # match both "Clang" and "AppleClang"
+            target_compile_options(${target} PRIVATE
+                -Wc++17-compat-pedantic
+                -Wc99-extensions
+                -Wgnu
+                -Wnon-gcc
+                -Wshadow-all
+            )
+        endif()
+
+        # Disabled warnings
         # arrow has a bunch of unused parameters in its headers.
         target_compile_options(${target} PRIVATE -Wno-unused-parameter)
-    endif()
 
-    # Enable this to fail on warnings.
-    # set_property(TARGET ${target} PROPERTY COMPILE_WARNING_AS_ERROR ON)
+        # CMAKE_COMPILE_WARNING_AS_ERROR is only directly supported starting in CMake `3.24`
+        # https://cmake.org/cmake/help/latest/prop_tgt/COMPILE_WARNING_AS_ERROR.html
+        if(${CMAKE_COMPILE_WARNING_AS_ERROR})
+            target_compile_options(${target} PRIVATE -Werror)
+        endif()
+    endif()
 endfunction()
 
 # Use makefiles on linux, otherwise it might use Ninja which isn't installed by default.

diff --git a/crates/re_types/source_hash.txt b/crates/re_types/source_hash.txt
diff --git a/crates/re_types_builder/src/codegen/cpp/mod.rs b/crates/re_types_builder/src/codegen/cpp/mod.rs
@@ -764,36 +764,47 @@ impl QuotedObject {
         };
 
         let copy_constructor = {
-            let copy_match_arms = obj
-                .fields
-                .iter()
-                .filter_map(|obj_field| {
-                    // Inferring from trivial destructability that we don't need to call the copy constructor is a little bit wonky,
-                    // but is typically the reason why we need to do this in the first place - if we'd always memcpy we'd get double-free errors.
-                    // (As with swap, we generously assume that objects are rellocatable)
-                    (!obj_field.typ.has_default_destructor(objects)).then(|| {
-                        let tag_ident = format_ident!("{}", obj_field.name);
-                        let field_ident =
-                            format_ident!("{}", crate::to_snake_case(&obj_field.name));
-                        Some(quote! {
-                            case detail::#tag_typename::#tag_ident: {
-                                _data.#field_ident = other._data.#field_ident;
-                                break;
-                            }
-                        })
-                    })
-                })
-                .collect_vec();
+            // Note that `switch` on an enum without handling all cases causes `-Wswitch-enum` warning!
+            let mut copy_match_arms = Vec::new();
+            let mut default_match_arms = Vec::new();
+            for obj_field in &obj.fields {
+                let tag_ident = format_ident!("{}", obj_field.name);
+                let case = quote!(case detail::#tag_typename::#tag_ident:);
+
+                // Inferring from trivial destructability that we don't need to call the copy constructor is a little bit wonky,
+                // but is typically the reason why we need to do this in the first place - if we'd always memcpy we'd get double-free errors.
+                // (As with swap, we generously assume that objects are rellocatable)
+                if obj_field.typ.has_default_destructor(objects) {
+                    default_match_arms.push(case);
+                } else {
+                    let field_ident = format_ident!("{}", crate::to_snake_case(&obj_field.name));
+                    copy_match_arms.push(quote! {
+                        #case {
+                            _data.#field_ident = other._data.#field_ident;
+                            break;
+                        }
+                    });
+                }
+            }
+
+            let trivial_memcpy = quote! {
+                const void* otherbytes = reinterpret_cast<const void*>(&other._data);
+                void* thisbytes = reinterpret_cast<void*>(&this->_data);
+                std::memcpy(thisbytes, otherbytes, sizeof(detail::#data_typename));
+            };
+
             if copy_match_arms.is_empty() {
                 quote!(#pascal_case_ident(const #pascal_case_ident& other) : _tag(other._tag) {
-                    memcpy(&this->_data, &other._data, sizeof(detail::#data_typename));
+                    #trivial_memcpy
                 })
             } else {
                 quote!(#pascal_case_ident(const #pascal_case_ident& other) : _tag(other._tag) {
                     switch (other._tag) {
                         #(#copy_match_arms)*
-                        default:
-                            memcpy(&this->_data, &other._data, sizeof(detail::#data_typename));
+
+                        case detail::#tag_typename::NONE:
+                        #(#default_match_arms)*
+                        #trivial_memcpy
                             break;
                     }
                 })
@@ -827,9 +838,11 @@ impl QuotedObject {
                                 #NEWLINE_TOKEN
                                 #swap_comment
                                 char temp[sizeof(#data_typename)];
-                                std::memcpy(temp, this, sizeof(#data_typename));
-                                std::memcpy(this, &other, sizeof(#data_typename));
-                                std::memcpy(&other, temp, sizeof(#data_typename));
+                                void* otherbytes = reinterpret_cast<void*>(&other);
+                                void* thisbytes = reinterpret_cast<void*>(this);
+                                std::memcpy(temp, thisbytes, sizeof(#data_typename));
+                                std::memcpy(thisbytes, otherbytes, sizeof(#data_typename));
+                                std::memcpy(otherbytes, temp, sizeof(#data_typename));
                             }
                         };
 
@@ -1241,7 +1254,10 @@ fn quote_fill_arrow_array_builder(
         let field = &obj.fields[0];
         if let Type::Object(fqname) = &field.typ {
             if field.is_nullable {
-                quote!(return arrow::Status::NotImplemented(("TODO(andreas) Handle nullable extensions"));)
+                quote! {
+                    (void)num_elements;
+                    return arrow::Status::NotImplemented(("TODO(andreas) Handle nullable extensions"));
+                }
             } else {
                 // Trivial forwarding to inner type.
                 let quoted_fqname = quote_fqname_as_type_path(includes, fqname);
@@ -1276,7 +1292,7 @@ fn quote_fill_arrow_array_builder(
                 quote! {
                     #(#fill_fields)*
                     #NEWLINE_TOKEN
-                    ARROW_RETURN_NOT_OK(builder->AppendValues(num_elements, nullptr));
+                    ARROW_RETURN_NOT_OK(builder->AppendValues(static_cast<int64_t>(num_elements), nullptr));
                 }
             }
             ObjectSpecifics::Union { .. } => {
@@ -1290,7 +1306,10 @@ fn quote_fill_arrow_array_builder(
                     let variant_name = format_ident!("{}", variant.name);
 
                     let variant_append = if variant.typ.is_plural() {
-                        quote! { return arrow::Status::NotImplemented("TODO(andreas): list types in unions are not yet supported");}
+                        quote! {
+                            (void)#variant_builder;
+                            return arrow::Status::NotImplemented("TODO(andreas): list types in unions are not yet supported");
+                        }
                     } else {
                         let variant_accessor = quote!(union_instance._data);
                         quote_append_single_field_to_builder(variant, &variant_builder, &variant_accessor, includes)
@@ -1307,10 +1326,10 @@ fn quote_fill_arrow_array_builder(
 
                 quote! {
                     #NEWLINE_TOKEN
-                    ARROW_RETURN_NOT_OK(#builder->Reserve(num_elements));
-                    for (auto elem_idx = 0; elem_idx < num_elements; elem_idx += 1) {
+                    ARROW_RETURN_NOT_OK(#builder->Reserve(static_cast<int64_t>(num_elements)));
+                    for (size_t elem_idx = 0; elem_idx < num_elements; elem_idx += 1) {
                         const auto& union_instance = elements[elem_idx];
-                        ARROW_RETURN_NOT_OK(#builder->Append(static_cast<uint8_t>(union_instance._tag)));
+                        ARROW_RETURN_NOT_OK(#builder->Append(static_cast<int8_t>(union_instance._tag)));
                         #NEWLINE_TOKEN
                         #NEWLINE_TOKEN
                         auto variant_index = static_cast<int>(union_instance._tag);
@@ -1356,9 +1375,13 @@ fn quote_append_field_to_builder(
             quote! {
                 auto #value_builder = static_cast<arrow::#value_builder_type*>(#builder->value_builder());
                 #NEWLINE_TOKEN #NEWLINE_TOKEN
-                ARROW_RETURN_NOT_OK(#builder->AppendValues(num_elements));
+                ARROW_RETURN_NOT_OK(#builder->AppendValues(static_cast<int64_t>(num_elements)));
                 static_assert(sizeof(elements[0].#field_name) == sizeof(elements[0]));
-                ARROW_RETURN_NOT_OK(#value_builder->AppendValues(#field_ptr_accessor, num_elements * #num_items_per_value, nullptr));
+                ARROW_RETURN_NOT_OK(#value_builder->AppendValues(
+                    #field_ptr_accessor,
+                    static_cast<int64_t>(num_elements * #num_items_per_value),
+                    nullptr)
+                );
             }
         } else {
             let value_reserve_factor = match &field.typ {
@@ -1376,8 +1399,8 @@ fn quote_append_field_to_builder(
 
             let setup = quote! {
                 auto #value_builder = static_cast<arrow::#value_builder_type*>(#builder->value_builder());
-                ARROW_RETURN_NOT_OK(#builder->Reserve(num_elements));
-                ARROW_RETURN_NOT_OK(#value_builder->Reserve(num_elements * #value_reserve_factor));
+                ARROW_RETURN_NOT_OK(#builder->Reserve(static_cast<int64_t>(num_elements)));
+                ARROW_RETURN_NOT_OK(#value_builder->Reserve(static_cast<int64_t>(num_elements * #value_reserve_factor)));
                 #NEWLINE_TOKEN #NEWLINE_TOKEN
             };
 
@@ -1397,7 +1420,7 @@ fn quote_append_field_to_builder(
             if field.is_nullable {
                 quote! {
                     #setup
-                    for (auto elem_idx = 0; elem_idx < num_elements; elem_idx += 1) {
+                    for (size_t elem_idx = 0; elem_idx < num_elements; elem_idx += 1) {
                         const auto& element = elements[elem_idx];
                         if (element.#field_name.has_value()) {
                             ARROW_RETURN_NOT_OK(#builder->Append());
@@ -1410,7 +1433,7 @@ fn quote_append_field_to_builder(
             } else {
                 quote! {
                     #setup
-                    for (auto elem_idx = 0; elem_idx < num_elements; elem_idx += 1) {
+                    for (size_t elem_idx = 0; elem_idx < num_elements; elem_idx += 1) {
                         const auto& element = elements[elem_idx];
                         ARROW_RETURN_NOT_OK(#builder->Append());
                         #append_value
@@ -1424,15 +1447,15 @@ fn quote_append_field_to_builder(
         let field_ptr_accessor = quote_field_ptr_access(&field.typ, quote!(elements->#field_name));
         quote! {
             static_assert(sizeof(*elements) == sizeof(elements->#field_name));
-            ARROW_RETURN_NOT_OK(#builder->AppendValues(#field_ptr_accessor, num_elements));
+            ARROW_RETURN_NOT_OK(#builder->AppendValues(#field_ptr_accessor, static_cast<int64_t>(num_elements)));
         }
     } else {
         let element_accessor = quote!(elements[elem_idx]);
         let single_append =
             quote_append_single_field_to_builder(field, builder, &element_accessor, includes);
         quote! {
-            ARROW_RETURN_NOT_OK(#builder->Reserve(num_elements));
-            for (auto elem_idx = 0; elem_idx < num_elements; elem_idx += 1) {
+            ARROW_RETURN_NOT_OK(#builder->Reserve(static_cast<int64_t>(num_elements)));
+            for (size_t elem_idx = 0; elem_idx < num_elements; elem_idx += 1) {
                 #single_append
             }
         }
@@ -1515,12 +1538,12 @@ fn quote_append_single_value_to_builder(
                 | ElementType::Float64 => {
                     let field_ptr_accessor = quote_field_ptr_access(typ, value_access);
                     quote! {
-                        ARROW_RETURN_NOT_OK(#value_builder->AppendValues(#field_ptr_accessor, #num_items_per_element, nullptr));
+                        ARROW_RETURN_NOT_OK(#value_builder->AppendValues(#field_ptr_accessor, static_cast<int64_t>(#num_items_per_element), nullptr));
                     }
                 }
                 ElementType::String => {
                     quote! {
-                        for (auto item_idx = 0; item_idx < #num_items_per_element; item_idx += 1) {
+                        for (size_t item_idx = 0; item_idx < #num_items_per_element; item_idx += 1) {
                             ARROW_RETURN_NOT_OK(#value_builder->Append(#value_access[item_idx]));
                         }
                     }
@@ -1728,26 +1751,31 @@ fn static_constructor_for_enum_type(
         // We need special casing for constructing arrays:
         let length = proc_macro2::Literal::usize_unsuffixed(*length);
 
-        let element_assignment = if elem_type.has_default_destructor(objects) {
+        let (element_assignment, typedef) = if elem_type.has_default_destructor(objects) {
             // Generate simpoler code for simple types:
-            quote! {
-                self._data.#snake_case_ident[i] = std::move(#snake_case_ident[i]);
-            }
+            (
+                quote! {
+                    self._data.#snake_case_ident[i] = std::move(#snake_case_ident[i]);
+                },
+                quote!(),
+            )
         } else {
             // We need to use placement-new since the union is in an uninitialized state here:
             hpp_includes.system.insert("new".to_owned()); // placement-new
-            quote! {
-                new (&self._data.#snake_case_ident[i]) TypeAlias(std::move(#snake_case_ident[i]));
-            }
+            let elem_type = quote_element_type(hpp_includes, elem_type);
+            (
+                quote! {
+                    new (&self._data.#snake_case_ident[i]) #elem_type(std::move(#snake_case_ident[i]));
+                },
+                quote!(typedef #elem_type TypeAlias;),
+            )
         };
 
-        let elem_type = quote_element_type(hpp_includes, elem_type);
-
         Method {
             docs,
             declaration,
             definition_body: quote! {
-                typedef #elem_type TypeAlias;
+                #typedef
                 #pascal_case_ident self;
                 self._tag = detail::#tag_typename::#tag_ident;
                 for (size_t i = 0; i < #length; i += 1) {

diff --git a/crates/rerun_c/src/rerun.h b/crates/rerun_c/src/rerun.h
@@ -19,12 +19,12 @@ extern "C" {
 /// Special value for `rr_recording_stream` methods to indicate the most appropriate
 /// globally available recording stream for recordings.
 /// (i.e. thread-local first, then global scope)
-#define RERUN_REC_STREAM_CURRENT_RECORDING 0xFFFFFFFF
+#define RERUN_REC_STREAM_CURRENT_RECORDING ((rr_recording_stream)0xFFFFFFFF)
 
 /// Special value for `rr_recording_stream` methods to indicate the most appropriate
 /// globally available recording stream for blueprints.
 /// (i.e. thread-local first, then global scope)
-#define RERUN_REC_STREAM_CURRENT_BLUEPRINT 0xFFFFFFFE
+#define RERUN_REC_STREAM_CURRENT_BLUEPRINT ((rr_recording_stream)0xFFFFFFFE)
 
 /// A unique handle for a recording stream.
 /// A recording stream handles everything related to logging data into Rerun.
@@ -48,7 +48,7 @@ extern "C" {
 ///
 /// TODO(andreas): The only way of having two instances of a `RecordingStream` is currently to
 /// set it as a the global.
-typedef int32_t rr_recording_stream;
+typedef uint32_t rr_recording_stream;
 
 struct rr_store_info {
     /// The user-chosen name of the application doing the logging.
@@ -64,7 +64,7 @@ struct rr_data_cell {
 
     /// The number of bytes in the `bytes` field.
     /// Must be a multiple of 8.
-    const uint64_t num_bytes;
+    uint64_t num_bytes;
 
     /// Data in the Arrow IPC encapsulated message format.
     ///

diff --git a/examples/cpp/minimal/CMakeLists.txt b/examples/cpp/minimal/CMakeLists.txt
@@ -11,6 +11,5 @@ endif()
 
 add_executable(rerun_example main.cpp)
 
-# TODO(andreas): Fix warnings and enable this.
-# set_default_warning_settings(rerun_example)
+set_default_warning_settings(rerun_example)
 target_link_libraries(rerun_example PRIVATE loguru::loguru rerun_sdk)
diff --git a/examples/cpp/minimal/build_and_run.sh b/examples/cpp/minimal/build_and_run.sh
@@ -5,11 +5,32 @@ script_path=$( cd "$(dirname "${BASH_SOURCE[0]}")" ; pwd -P )
 cd "$script_path/../../.."
 set -x
 
+WERROR=false
+
+while test $# -gt 0; do
+  case "$1" in
+    --werror)
+      shift
+      WERROR=true
+      ;;
+
+    *)
+      echo "Unknown option: $1"
+      exit 1
+      ;;
+  esac
+done
+
+
 num_threads=$(getconf _NPROCESSORS_ONLN)
 
 mkdir -p build
 pushd build
-    cmake -DCMAKE_BUILD_TYPE=Debug ..
+    if [ ${WERROR} = true ]; then
+        cmake -DCMAKE_BUILD_TYPE=Debug -DCMAKE_COMPILE_WARNING_AS_ERROR=ON ..
+    else
+        cmake -DCMAKE_BUILD_TYPE=Debug ..
+    fi
     cmake --build . --config Debug --target rerun_example -j ${num_threads}
 popd