Enable warnings (#21)

* Enable warnings

* Fix some warnings

* Try to enable deprecation warnings

* Ignore warnings in VRS source code

* Try that again

* Try another fix

* I hate CMAKE

Author: emilk

CMakeLists.txt

@@ -1,6 +1,9 @@
 cmake_minimum_required(VERSION 3.16...3.27)
 
-project(rerun_vrs_example LANGUAGES CXX)
+set(CMAKE_COMPILE_WARNING_AS_ERROR OFF) # TODO(emilk): If we turn this ON, VRS fails to compile
+
+set(TARGET_NAME rerun_vrs_example)
+project(${TARGET_NAME} LANGUAGES CXX)
 
 set(CMAKE_EXPORT_COMPILE_COMMANDS ON)
 
@@ -22,6 +25,98 @@ FetchContent_MakeAvailable(vrslib)
 
 find_package(fmt REQUIRED)
 
-add_executable(rerun_vrs_example src/main.cpp src/frame_player.cpp src/imu_player.cpp)
-target_link_libraries(rerun_vrs_example rerun_sdk vrslib vrs_utils fmt)
-target_include_directories(rerun_vrs_example PRIVATE src)
+add_executable(${TARGET_NAME} src/main.cpp src/frame_player.cpp src/imu_player.cpp)
+target_link_libraries(${TARGET_NAME} rerun_sdk vrslib vrs_utils fmt)
+target_include_directories(${TARGET_NAME} PRIVATE src)
+target_include_directories(${TARGET_NAME} SYSTEM PRIVATE ${rerun_sdk_SOURCE_DIR}) # Ignore warnings in Rerun SDK headers
+target_include_directories(${TARGET_NAME} SYSTEM PRIVATE ${vrslib_SOURCE_DIR}) # Ignore warnings in VRS headers
+
+
+if(MSVC)
+    # TODO(andreas): Try to enable /Wall
+    target_compile_options(${TARGET_NAME} PRIVATE /W4)
+
+    target_compile_options(${TARGET_NAME} PRIVATE /we4996) # Using deprecated functions is an error
+
+    if(BUILD_SHARED_LIBS)
+        # If we are building as shared libs, we are going to have to disable the C4251
+        # warning, as it would trigger for any datatype derived from a STL class
+        # See also https://github.com/protocolbuffers/protobuf/blob/v26.1/cmake/README.md#notes-on-compiler-warnings
+        # We need also to make it public, otherwise downstream code will be flooded by c4251 warnings
+        target_compile_options(${TARGET_NAME} PUBLIC /wd4251)
+    endif()
+
+    # 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_NAME} PRIVATE /WX
+            /w15038 # Initialization order. https://learn.microsoft.com/en-us/cpp/error-messages/compiler-warnings/c5038
+        )
+    endif()
+else()
+    # Enabled warnings.
+    target_compile_options(${TARGET_NAME} PRIVATE
+        -Wall
+        -Wcast-align
+        -Wcast-qual
+        -Wdeprecated
+        -Wdeprecated-declarations
+        -Werror=deprecated-declarations
+        -Wextra
+        -Wformat=2
+        -Wmissing-include-dirs
+        -Wnull-dereference
+        -Wold-style-cast
+        -Wpedantic
+        -Wpointer-arith
+        -Wshadow
+        -Wswitch-enum
+        -Wunreachable-code
+        -Wvla
+    )
+
+    if(CMAKE_CXX_COMPILER_ID MATCHES "Clang") # match both "Clang" and "AppleClang"
+        # TODO(emilk): enable some hardening flags from https://best.openssf.org/Compiler-Hardening-Guides/Compiler-Options-Hardening-Guide-for-C-and-C++.html
+        target_compile_options(${TARGET_NAME} PRIVATE
+            -Wc++17-compat-pedantic
+            -Wc++20-compat-pedantic
+            -Wc99-extensions
+            -Weverything
+            -Wgnu
+            -Wnon-gcc
+            -Wpre-c2x-compat-pedantic
+            -Wshadow-all
+
+            # Turn off some warning that -Weverything turns on:
+            -Wno-c++98-compat
+            -Wno-c++98-compat-pedantic
+            -Wno-covered-switch-default # We often add a `default:` case out of paranoia
+            -Wno-ctad-maybe-unsupported
+            -Wno-disabled-macro-expansion
+            -Wno-documentation
+            -Wno-documentation-unknown-command
+            -Wno-double-promotion # float->double is nothing to whine about
+            -Wno-exit-time-destructors
+            -Wno-float-equal # comparing floats is fine
+            -Wno-global-constructors
+            -Wno-missing-prototypes
+            -Wno-padded
+            -Wno-reserved-id-macro
+            -Wno-reserved-identifier
+            -Wno-unused-macros
+            -Wno-unsafe-buffer-usage # Not sure why we need this, but we do.
+            -Wno-unknown-warning-option # Otherwise older clang will complain about `-Wno-unsafe-buffer-usage`
+        )
+    endif()
+
+    # 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_NAME} PRIVATE -Werror)
+    endif()
+
+    if(CMAKE_BUILD_TYPE STREQUAL "Debug")
+        # Improve stack traces:
+        target_compile_options(${TARGET_NAME} PRIVATE -g -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer -fno-optimize-sibling-calls)
+    endif()
+endif()

src/frame_player.cpp

@@ -57,7 +57,7 @@ namespace rerun_vrs {
             auto& config = getExpectedLayout<FrameNumberDataLayout>(layout, block_index);
             uint64_t frame_number;
             if (config.frameNumber.get(frame_number)) {
-                _rec->set_time_sequence("frame_number", frame_number);
+                _rec->set_time_sequence("frame_number", static_cast<int64_t>(frame_number));
             }
 
             // this is meta data per record and changes over time

src/frame_player.hpp

@@ -36,8 +36,8 @@ namespace rerun_vrs {
             override;
 
       private:
-        std::shared_ptr<const rerun::RecordingStream> _rec;
         vrs::StreamId _id;
+        std::shared_ptr<const rerun::RecordingStream> _rec;
         std::string _entity_path;
         bool _enabled{true};
     };

src/imu_player.hpp

@@ -38,8 +38,8 @@ namespace rerun_vrs {
         void log_gyroscope(const std::array<float, 3>& gyroRadSec);
         void log_magnetometer(const std::array<float, 3>& magTesla);
 
-        std::shared_ptr<const rerun::RecordingStream> _rec;
         vrs::StreamId _id;
+        std::shared_ptr<const rerun::RecordingStream> _rec;
         std::string _entity_path;
         bool _enabled{true};
         bool _has_accelerometer;