Add CMake OTELCPP_MAINTAINER_MODE (#1650)

.github/workflows/ci.yml

@@ -23,6 +23,52 @@ jobs:
         sudo ./ci/setup_thrift.sh
         ./ci/do_ci.sh cmake.test
 
+  cmake_gcc_maintainer_test:
+    name: CMake gcc (maintainer mode)
+    runs-on: ubuntu-latest
+    steps:
+    - uses: actions/checkout@v3
+      with:
+        submodules: 'recursive'
+    - name: setup
+      run: |
+        sudo CC=/usr/bin/gcc-10 CXX=/usr/bin/g++-10 ./ci/setup_cmake.sh
+        sudo CC=/usr/bin/gcc-10 CXX=/usr/bin/g++-10 ./ci/setup_ci_environment.sh
+    - name: run cmake gcc (maintainer mode)
+      run: |
+        sudo CC=/usr/bin/gcc-10 CXX=/usr/bin/g++-10 ./ci/setup_thrift.sh
+        CC=/usr/bin/gcc-10 CXX=/usr/bin/g++-10 ./ci/do_ci.sh cmake.maintainer.test
+
+  cmake_clang_maintainer_test:
+    name: CMake clang (maintainer mode)
+    runs-on: ubuntu-latest
+    steps:
+    - uses: actions/checkout@v3
+      with:
+        submodules: 'recursive'
+    - name: setup
+      run: |
+        sudo CC=/usr/bin/clang-12 CXX=/usr/bin/clang++-12 ./ci/setup_cmake.sh
+        sudo CC=/usr/bin/clang-12 CXX=/usr/bin/clang++-12 ./ci/setup_ci_environment.sh
+    - name: run cmake clang (maintainer mode)
+      run: |
+        sudo CC=/usr/bin/clang-12 CXX=/usr/bin/clang++-12 ./ci/setup_thrift.sh
+        CC=/usr/bin/clang-12 CXX=/usr/bin/clang++-12 ./ci/do_ci.sh cmake.maintainer.test
+
+  cmake_msvc_maintainer_test:
+    name: CMake msvc (maintainer mode)
+    runs-on: windows-latest
+    steps:
+    - uses: actions/checkout@v3
+      with:
+        submodules: 'recursive'
+    - name: setup
+      run: |
+        ./ci/setup_windows_cmake.ps1
+        ./ci/setup_windows_ci_environment.ps1
+    - name: run tests
+      run: ./ci/do_ci.ps1 cmake.maintainer.test
+
   cmake_deprecated_metrics_test:
     name: CMake test (without otlp-exporter and with deprecated metrics)
     runs-on: ubuntu-latest

CHANGELOG.md

@@ -15,6 +15,8 @@ Increment the:
 
 ## [Unreleased]
 
+* [BUILD] Add CMake OTELCPP_MAINTAINER_MODE [#1650](https://github.com/open-telemetry/opentelemetry-cpp/pull/1650)
+
 ## [1.6.1] 2022-09-22
 
 * [BUILD] Upgrade opentelemetry-proto to v0.19.0 [#1579](https://github.com/open-telemetry/opentelemetry-cpp/pull/1579)

CMakeLists.txt

@@ -166,6 +166,8 @@ option(BUILD_TESTING "Whether to enable tests" ON)
 
 option(BUILD_W3CTRACECONTEXT_TEST "Whether to build w3c trace context" OFF)
 
+option(OTELCPP_MAINTAINER_MODE "Build in maintainer mode (-Wall -Werror)" OFF)
+
 if(WIN32)
   if(BUILD_TESTING)
     if(MSVC)
@@ -358,6 +360,88 @@ if((NOT WITH_API_ONLY) AND USE_NLOHMANN_JSON)
   endif()
 endif()
 
+if(OTELCPP_MAINTAINER_MODE)
+  if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
+    message("Building with gcc in maintainer mode.")
+
+    add_compile_options(-Wall)
+    add_compile_options(-Werror)
+    add_compile_options(-Wextra)
+
+    # Tested with GCC 9.4 on github.
+    if(CMAKE_CXX_COMPILER_VERSION VERSION_GREATER_EQUAL 9.4)
+      message("Building with additional warnings for gcc.")
+
+      # Relaxed warnings
+
+      # Enforced warnings
+
+      # C++ options only
+      add_compile_options($<$<STREQUAL:$<COMPILE_LANGUAGE>,CXX>:-Wextra-semi>)
+      add_compile_options(
+        $<$<STREQUAL:$<COMPILE_LANGUAGE>,CXX>:-Woverloaded-virtual>)
+      add_compile_options(
+        $<$<STREQUAL:$<COMPILE_LANGUAGE>,CXX>:-Wsuggest-override>)
+
+      # C and C++
+      add_compile_options(-Wcast-qual)
+      add_compile_options(-Wformat-security)
+      add_compile_options(-Wlogical-op)
+      add_compile_options(-Wmissing-include-dirs)
+      add_compile_options(-Wstringop-truncation)
+      add_compile_options(-Wundef)
+      add_compile_options(-Wvla)
+    endif()
+  elseif(CMAKE_CXX_COMPILER_ID STREQUAL "Clang")
+    message("Building with clang in maintainer mode.")
+
+    add_compile_options(-Wall)
+    add_compile_options(-Werror)
+    add_compile_options(-Wextra)
+
+    # Tested with Clang 11.0 on github.
+    if(CMAKE_CXX_COMPILER_VERSION VERSION_GREATER_EQUAL 11.0)
+      message("Building with additional warnings for clang.")
+
+      # Relaxed warnings
+      add_compile_options(-Wno-error=unused-private-field)
+
+      # Enforced warnings
+      add_compile_options(-Wcast-qual)
+      add_compile_options(-Wconditional-uninitialized)
+      add_compile_options(-Wextra-semi)
+      add_compile_options(-Wformat-security)
+      add_compile_options(-Wheader-hygiene)
+      add_compile_options(-Winconsistent-missing-destructor-override)
+      add_compile_options(-Winconsistent-missing-override)
+      add_compile_options(-Wnewline-eof)
+      add_compile_options(-Wnon-virtual-dtor)
+      add_compile_options(-Woverloaded-virtual)
+      add_compile_options(-Wrange-loop-analysis)
+      add_compile_options(-Wundef)
+      add_compile_options(-Wundefined-reinterpret-cast)
+      add_compile_options(-Wvla)
+    endif()
+  elseif(CMAKE_CXX_COMPILER_ID STREQUAL "MSVC")
+    message("Building with msvc in maintainer mode.")
+
+    add_compile_options(/WX)
+    add_compile_options(/W4)
+
+    # Relaxed warnings
+    add_compile_options(/wd4100)
+    add_compile_options(/wd4125)
+    add_compile_options(/wd4566)
+    add_compile_options(/wd4127)
+    add_compile_options(/wd4512)
+    add_compile_options(/wd4267)
+
+    # Enforced warnings
+  elseif()
+    message(FATAL_ERROR "Building with unknown compiler in maintainer mode.")
+  endif()
+endif(OTELCPP_MAINTAINER_MODE)
+
 list(APPEND CMAKE_PREFIX_PATH "${CMAKE_BINARY_DIR}")
 
 include(CTest)

api/test/nostd/unique_ptr_test.cc

@@ -122,7 +122,7 @@ TEST(UniquePtrTest, Reset)
 {
   bool was_destructed1;
   unique_ptr<A> ptr{new A{was_destructed1}};
-  bool was_destructed2;
+  bool was_destructed2 = true;
   ptr.reset(new A{was_destructed2});
   EXPECT_TRUE(was_destructed1);
   EXPECT_FALSE(was_destructed2);

api/test/trace/span_benchmark.cc

@@ -115,10 +115,10 @@ void BM_SpanCreationWitContextPropagation(benchmark::State &state)
         nostd::shared_ptr<trace_api::Span>(new trace_api::DefaultSpan(outer_span_context));
     trace_api::SetSpan(current_ctx, outer_span);
     auto inner_child = tracer->StartSpan("inner");
-    auto scope       = tracer->WithActiveSpan(inner_child);
+    auto inner_scope = tracer->WithActiveSpan(inner_child);
     {
       auto innermost_child = tracer->StartSpan("innermost");
-      auto scope           = tracer->WithActiveSpan(innermost_child);
+      auto innermost_scope = tracer->WithActiveSpan(innermost_child);
       innermost_child->End();
     }
     inner_child->End();

ci/README.md

@@ -4,6 +4,7 @@ CI tests can be run on docker by invoking the script `./ci/run_docker.sh
 ./ci/do_ci.sh {TARGET}` where the targets are:
 
 * `cmake.test`: build cmake targets and run tests.
+* `cmake.maintainer.test`: build with cmake and test, in maintainer mode.
 * `cmake.legacy.test`: build cmake targets with gcc 4.8 and run tests.
 * `cmake.c++20.test`: build cmake targets with the C++20 standard and run tests.
 * `cmake.test_example_plugin`: build and test an example OpenTelemetry plugin.

ci/do_ci.ps1

@@ -48,6 +48,27 @@ switch ($action) {
       exit $exit
     }
   }
+  "cmake.maintainer.test" {
+    cd "$BUILD_DIR"
+    cmake $SRC_DIR `
+      -DOTELCPP_MAINTAINER_MODE=ON `
+      -DVCPKG_TARGET_TRIPLET=x64-windows `
+      "-DCMAKE_TOOLCHAIN_FILE=$VCPKG_DIR/scripts/buildsystems/vcpkg.cmake"
+    $exit = $LASTEXITCODE
+    if ($exit -ne 0) {
+      exit $exit
+    }
+    cmake --build .
+    $exit = $LASTEXITCODE
+    if ($exit -ne 0) {
+      exit $exit
+    }
+    ctest -C Debug
+    $exit = $LASTEXITCODE
+    if ($exit -ne 0) {
+      exit $exit
+    }
+  }
   "cmake.with_async_export.test" {
     cd "$BUILD_DIR"
     cmake $SRC_DIR `

ci/do_ci.sh

@@ -89,6 +89,22 @@ if [[ "$1" == "cmake.test" ]]; then
   make
   make test
   exit 0
+elif [[ "$1" == "cmake.maintainer.test" ]]; then
+  cd "${BUILD_DIR}"
+  rm -rf *
+  cmake -DCMAKE_BUILD_TYPE=Debug  \
+        -DWITH_PROMETHEUS=ON \
+        -DWITH_ZIPKIN=ON \
+        -DWITH_JAEGER=ON \
+        -DWITH_ELASTICSEARCH=ON \
+        -DWITH_LOGS_PREVIEW=ON \
+        -DWITH_METRICS_PREVIEW=OFF \
+        -DWITH_ASYNC_EXPORT_PREVIEW=ON \
+        -DOTELCPP_MAINTAINER_MODE=ON \
+        "${SRC_DIR}"
+  make -k
+  make test
+  exit 0
 elif [[ "$1" == "cmake.with_async_export.test" ]]; then
   cd "${BUILD_DIR}"
   rm -rf *

sdk/include/opentelemetry/sdk/metrics/exemplar/histogram_exemplar_reservoir.h

@@ -55,7 +55,8 @@ class HistogramExemplarReservoir : public FixedSizeExemplarReservoir
                               const MetricAttributes & /* attributes */,
                               const opentelemetry::context::Context & /* context */) override
     {
-      for (size_t i = 0; i < boundaries_.size(); ++i)
+      int max_size = boundaries_.size();
+      for (int i = 0; i < max_size; ++i)
       {
         if (value <= boundaries_[i])
         {

sdk/test/metrics/async_metric_storage_test.cc

@@ -77,24 +77,24 @@ TEST_P(WritableMetricStorageTestFixture, TestAggregation)
   storage.RecordLong(measurements1,
                      opentelemetry::common::SystemTimestamp(std::chrono::system_clock::now()));
 
-  storage.Collect(collector.get(), collectors, sdk_start_ts, collection_ts,
-                  [&](const MetricData data) {
-                    for (auto data_attr : data.point_data_attr_)
-                    {
-                      auto data = opentelemetry::nostd::get<SumPointData>(data_attr.point_data);
-                      if (opentelemetry::nostd::get<std::string>(
-                              data_attr.attributes.find("RequestType")->second) == "GET")
-                      {
-                        EXPECT_EQ(opentelemetry::nostd::get<long>(data.value_), get_count1);
-                      }
-                      else if (opentelemetry::nostd::get<std::string>(
-                                   data_attr.attributes.find("RequestType")->second) == "PUT")
-                      {
-                        EXPECT_EQ(opentelemetry::nostd::get<long>(data.value_), put_count1);
-                      }
-                    }
-                    return true;
-                  });
+  storage.Collect(
+      collector.get(), collectors, sdk_start_ts, collection_ts, [&](const MetricData &metric_data) {
+        for (const auto &data_attr : metric_data.point_data_attr_)
+        {
+          const auto &data = opentelemetry::nostd::get<SumPointData>(data_attr.point_data);
+          if (opentelemetry::nostd::get<std::string>(
+                  data_attr.attributes.find("RequestType")->second) == "GET")
+          {
+            EXPECT_EQ(opentelemetry::nostd::get<long>(data.value_), get_count1);
+          }
+          else if (opentelemetry::nostd::get<std::string>(
+                       data_attr.attributes.find("RequestType")->second) == "PUT")
+          {
+            EXPECT_EQ(opentelemetry::nostd::get<long>(data.value_), put_count1);
+          }
+        }
+        return true;
+      });
   // subsequent recording after collection shouldn't fail
   // monotonic increasing values;
   long get_count2 = 50l;
@@ -105,10 +105,10 @@ TEST_P(WritableMetricStorageTestFixture, TestAggregation)
   storage.RecordLong(measurements2,
                      opentelemetry::common::SystemTimestamp(std::chrono::system_clock::now()));
   storage.Collect(
-      collector.get(), collectors, sdk_start_ts, collection_ts, [&](const MetricData data) {
-        for (auto data_attr : data.point_data_attr_)
+      collector.get(), collectors, sdk_start_ts, collection_ts, [&](const MetricData &metric_data) {
+        for (const auto &data_attr : metric_data.point_data_attr_)
         {
-          auto data = opentelemetry::nostd::get<SumPointData>(data_attr.point_data);
+          const auto &data = opentelemetry::nostd::get<SumPointData>(data_attr.point_data);
           if (opentelemetry::nostd::get<std::string>(
                   data_attr.attributes.find("RequestType")->second) == "GET")
           {
@@ -171,8 +171,8 @@ TEST_P(WritableMetricStorageTestObservableGaugeFixture, TestAggregation)
                      opentelemetry::common::SystemTimestamp(std::chrono::system_clock::now()));
 
   storage.Collect(
-      collector.get(), collectors, sdk_start_ts, collection_ts, [&](const MetricData data) {
-        for (auto data_attr : data.point_data_attr_)
+      collector.get(), collectors, sdk_start_ts, collection_ts, [&](const MetricData metric_data) {
+        for (auto data_attr : metric_data.point_data_attr_)
         {
           auto data = opentelemetry::nostd::get<LastValuePointData>(data_attr.point_data);
           if (opentelemetry::nostd::get<std::string>(data_attr.attributes.find("CPU")->second) ==
@@ -197,8 +197,8 @@ TEST_P(WritableMetricStorageTestObservableGaugeFixture, TestAggregation)
   storage.RecordLong(measurements2,
                      opentelemetry::common::SystemTimestamp(std::chrono::system_clock::now()));
   storage.Collect(
-      collector.get(), collectors, sdk_start_ts, collection_ts, [&](const MetricData data) {
-        for (auto data_attr : data.point_data_attr_)
+      collector.get(), collectors, sdk_start_ts, collection_ts, [&](const MetricData metric_data) {
+        for (auto data_attr : metric_data.point_data_attr_)
         {
           auto data = opentelemetry::nostd::get<LastValuePointData>(data_attr.point_data);
           if (opentelemetry::nostd::get<std::string>(data_attr.attributes.find("CPU")->second) ==