chore(msvc): fix compilation warnings (#188)

Changes: - Treat warnings as error in CI builds for MSVC.
DataDog · Feb 18, 2025 · dc5781d · dc5781d
1 parent 1a632bc
commit dc5781d
Show file tree

Hide file tree

Showing 18 changed files with 128 additions and 145 deletions.
diff --git a/.circleci/config.yml b/.circleci/config.yml
@@ -116,7 +116,7 @@ jobs:
         name: Building
         command: |
           & 'C:\\Program Files\\Microsoft Visual Studio\\2022\\Community\\Common7\\Tools\\Launch-VsDevShell.ps1' -arch << parameters.arch >> 
-          cmake -B build -DCMAKE_BUILD_TYPE=Debug -DBUILD_SHARED_LIBS=OFF -DDD_TRACE_STATIC_CRT=1 -DDD_TRACE_BUILD_TESTING=1 -G Ninja .
+          cmake --preset=ci-windows -B build -DCMAKE_BUILD_TYPE=Debug .
           cmake --build build -j $env:MAKE_JOB_COUNT -v
     - run:
         name: Testing

diff --git a/CMakePresets.json b/CMakePresets.json
@@ -0,0 +1,17 @@
+{
+  "version": 8,
+  "$schema": "https://cmake.org/cmake/help/latest/_downloads/3e2d73bff478d88a7de0de736ba5e361/schema.json",
+  "configurePresets": [
+    {
+      "name": "ci-windows",
+      "displayName": "CI Windows",
+      "generator": "Ninja",
+      "cacheVariables": {
+        "CMAKE_COMPILE_WARNING_AS_ERROR": "1",
+        "BUILD_SHARED_LIBS": "OFF",
+        "DD_TRACE_STATIC_CRT": "1",
+        "DD_TRACE_BUILD_TESTING": "1"
+      }
+    }
+  ]
+}
diff --git a/src/datadog/string_util.cpp b/src/datadog/string_util.cpp
@@ -31,14 +31,14 @@ std::string join(const Sequence& elements, StringView separator,
 
 void to_lower(std::string& text) {
   std::transform(text.begin(), text.end(), text.begin(),
-                 [](unsigned char ch) { return std::tolower(ch); });
+                 [](auto c) { return (char)std::tolower(c); });
 }
 
 std::string to_lower(StringView sv) {
   std::string s;
   s.reserve(sv.size());
   std::transform(sv.begin(), sv.end(), std::back_inserter(s),
-                 [](char c) { return std::tolower(c); });
+                 [](auto c) { return (char)std::tolower(c); });
 
   return s;
 }
@@ -47,7 +47,7 @@ std::string to_upper(StringView sv) {
   std::string s;
   s.reserve(sv.size());
   std::transform(sv.begin(), sv.end(), std::back_inserter(s),
-                 [](char c) { return std::toupper(c); });
+                 [](auto c) { return (char)std::toupper(c); });
 
   return s;
 }

diff --git a/src/datadog/w3c_propagation.cpp b/src/datadog/w3c_propagation.cpp
@@ -111,7 +111,7 @@ Optional<std::string> extract_traceparent(ExtractedData& result,
       parse_uint64(StringView(traceparent.data() + beg, 2), 16);
   if (maybe_trace_flags.if_error()) return "malformed_traceflags";
 
-  result.sampling_priority = *maybe_trace_flags & 0x01;
+  result.sampling_priority = static_cast<int>(*maybe_trace_flags & 0x01);
 
   return nullopt;
 }

diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
@@ -52,6 +52,13 @@ target_include_directories(tests
     ${CMAKE_SOURCE_DIR}/include/datadog
 )
 
+# Enable Catch2 std stringification 
+# <https://github.com/catchorg/Catch2/blob/devel/docs/configuration.md#enabling-stringification>
+target_compile_definitions(tests
+  PUBLIC
+    CATCH_CONFIG_ENABLE_ALL_STRINGMAKERS
+)
+
 target_link_libraries(tests 
   PRIVATE
     # TODO: Remove dependency on libcurl

diff --git a/test/mocks/collectors.h b/test/mocks/collectors.h
@@ -61,7 +61,7 @@ struct MockCollectorWithResponse : public MockCollector {
 };
 
 struct PriorityCountingCollector : public Collector {
-  std::map<int, std::size_t> sampling_priority_count;
+  std::map<double, std::size_t> sampling_priority_count;
 
   Expected<void> send(std::vector<std::unique_ptr<SpanData>>&& spans,
                       const std::shared_ptr<TraceSampler>&) override {

diff --git a/test/mocks/loggers.h b/test/mocks/loggers.h
@@ -78,11 +78,11 @@ struct MockLogger : public Logger {
     entries.push_back(Entry{Entry::DD_ERROR, std::string(message)});
   }
 
-  int error_count() const { return count(Entry::DD_ERROR); }
+  auto error_count() const { return count(Entry::DD_ERROR); }
 
-  int startup_count() const { return count(Entry::STARTUP); }
+  auto startup_count() const { return count(Entry::STARTUP); }
 
-  int count(Entry::Kind kind) const {
+  size_t count(Entry::Kind kind) const {
     std::lock_guard<std::mutex> lock{mutex};
     return std::count_if(
         entries.begin(), entries.end(),

diff --git a/test/remote_config/test_remote_config.cpp b/test/remote_config/test_remote_config.cpp
@@ -357,14 +357,14 @@ REMOTE_CONFIG_TEST("response processing") {
           ]
       })";
 
-      const auto response_json =
+      const auto new_response_json =
           nlohmann::json::parse(/* input = */ new_rc_response,
                                 /* parser_callback = */ nullptr,
                                 /* allow_exceptions = */ false);
 
-      REQUIRE(!response_json.is_discarded());
+      REQUIRE(!new_response_json.is_discarded());
 
-      rc.process_response(response_json);
+      rc.process_response(new_response_json);
 
       CHECK(tracing_listener->count_on_update == 2);
       CHECK(tracing_listener->count_on_revert == 0);
@@ -398,14 +398,14 @@ REMOTE_CONFIG_TEST("response processing") {
             ]
         })";
 
-        const auto response_json =
+        const auto new_response_json =
             nlohmann::json::parse(/* input = */ rc_partial_revert_response,
                                   /* parser_callback = */ nullptr,
                                   /* allow_exceptions = */ false);
 
-        REQUIRE(!response_json.is_discarded());
+        REQUIRE(!new_response_json.is_discarded());
 
-        rc.process_response(response_json);
+        rc.process_response(new_response_json);
 
         CHECK(tracing_listener->count_on_update == 1);
         CHECK(tracing_listener->count_on_revert == 1);
@@ -422,14 +422,14 @@ REMOTE_CONFIG_TEST("response processing") {
           "target_files": [{}]
         })";
 
-        const auto response_json =
+        const auto new_response_json =
             nlohmann::json::parse(/* input = */ rc_revert_response,
                                   /* parser_callback = */ nullptr,
                                   /* allow_exceptions = */ false);
 
-        REQUIRE(!response_json.is_discarded());
+        REQUIRE(!new_response_json.is_discarded());
 
-        rc.process_response(response_json);
+        rc.process_response(new_response_json);
 
         CHECK(tracing_listener->count_on_update == 1);
         CHECK(tracing_listener->count_on_revert == 1);

diff --git a/test/system-tests/utils.h b/test/system-tests/utils.h
@@ -12,7 +12,7 @@ namespace dd = datadog::tracing;
 
 inline std::string tolower(std::string s) {
   std::transform(s.begin(), s.end(), s.begin(),
-                 [](unsigned char c) { return std::tolower(c); });
+                 [](auto c) { return (char)std::tolower(c); });
   return s;
 }
 

diff --git a/test/test.cpp b/test/test.cpp
@@ -1,29 +1,5 @@
 #include "test.h"
 
-namespace std {
-
-std::ostream& operator<<(
-    std::ostream& stream,
-    const std::pair<const std::string, std::string>& item) {
-  return stream << '{' << item.first << ", " << item.second << '}';
-}
-
-std::ostream& operator<<(
-    std::ostream& stream,
-    const datadog::tracing::Optional<datadog::tracing::StringView>& item) {
-  return stream << item.value_or("<nullopt>");
-}
-
-std::ostream& operator<<(std::ostream& stream,
-                         const std::optional<int>& maybe) {
-  if (maybe) {
-    return stream << *maybe;
-  }
-  return stream << "<nullopt>";
-}
-
-}  // namespace std
-
 namespace datadog {
 namespace tracing {
 

diff --git a/test/test.h b/test/test.h
@@ -19,18 +19,6 @@
 
 #include "catch.hpp"
 
-namespace std {
-
-std::ostream& operator<<(std::ostream& stream,
-                         const std::pair<const std::string, std::string>& item);
-
-std::ostream& operator<<(std::ostream& stream,
-                         const std::optional<std::string_view>& maybe);
-
-std::ostream& operator<<(std::ostream& stream, const std::optional<int>& maybe);
-
-}  // namespace std
-
 namespace datadog {
 namespace tracing {
 

diff --git a/test/test_span.cpp b/test/test_span.cpp
@@ -67,11 +67,11 @@ TEST_CASE("set_tag") {
 
   SECTION("tags can be overwritten") {
     {
-      SpanConfig config;
-      config.tags = {{"color", "purple"},
-                     {"turtle.depth", "all the way down"},
-                     {"_dd.tag", "written"}};
-      auto span = tracer.create_span(config);
+      SpanConfig span_config;
+      span_config.tags = {{"color", "purple"},
+                          {"turtle.depth", "all the way down"},
+                          {"_dd.tag", "written"}};
+      auto span = tracer.create_span(span_config);
       span.set_tag("color", "green");
       span.set_tag("bonus", "applied");
       span.set_tag("_dd.tag", "overwritten");
@@ -119,13 +119,13 @@ TEST_CASE("lookup_tag") {
   }
 
   SECTION("lookup after config") {
-    SpanConfig config;
-    config.tags = {
+    SpanConfig span_config;
+    span_config.tags = {
         {"color", "purple"},
         {"turtle.depth", "all the way down"},
         {"_dd.tag", "found"},
     };
-    auto span = tracer.create_span(config);
+    auto span = tracer.create_span(span_config);
 
     REQUIRE(span.lookup_tag("color") == "purple");
     REQUIRE(span.lookup_tag("turtle.depth") == "all the way down");
@@ -150,9 +150,9 @@ TEST_CASE("remove_tag") {
   }
 
   SECTION("after removal, lookup yields null") {
-    SpanConfig config;
-    config.tags = {{"mayfly", "carpe diem"}, {"_dd.mayfly", "carpe diem"}};
-    auto span = tracer.create_span(config);
+    SpanConfig span_config;
+    span_config.tags = {{"mayfly", "carpe diem"}, {"_dd.mayfly", "carpe diem"}};
+    auto span = tracer.create_span(span_config);
     span.set_tag("foo", "bar");
 
     span.remove_tag("mayfly");
@@ -294,9 +294,9 @@ TEST_CASE("span duration") {
 
   SECTION("start time is adjustable") {
     {
-      SpanConfig config;
-      config.start = default_clock() - std::chrono::seconds(3);
-      auto span = tracer.create_span(config);
+      SpanConfig span_config;
+      span_config.start = default_clock() - std::chrono::seconds(3);
+      auto span = tracer.create_span(span_config);
       (void)span;
     }
 

diff --git a/test/test_span_sampler.cpp b/test/test_span_sampler.cpp
@@ -103,8 +103,6 @@ SpanSamplerConfig::Rule by_name_and_tags(
   return rule;
 }
 
-const auto x = nullopt;
-
 }  // namespace
 
 TEST_CASE("span rules matching") {
@@ -121,34 +119,34 @@ TEST_CASE("span rules matching") {
       {"no rules → no span sampling tags", {}, {}, {}, {}, {}},
       {"match by service",
        {by_service("testsvc")},
-       {8, 1.0, x},
-       {8, 1.0, x},
-       {8, 1.0, x},
-       {8, 1.0, x}},
+       {8, 1.0, nullopt},
+       {8, 1.0, nullopt},
+       {8, 1.0, nullopt},
+       {8, 1.0, nullopt}},
       {"match by name",
        {by_name("sibling")},
-       {x, x, x},
-       {x, x, x},
-       {8, 1.0, x},
-       {x, x, x}},
+       {nullopt, nullopt, nullopt},
+       {nullopt, nullopt, nullopt},
+       {8, 1.0, nullopt},
+       {nullopt, nullopt, nullopt}},
       {"match by resource",
        {by_resource("office")},
-       {x, x, x},
-       {8, 1.0, x},
-       {x, x, x},
-       {x, x, x}},
+       {nullopt, nullopt, nullopt},
+       {8, 1.0, nullopt},
+       {nullopt, nullopt, nullopt},
+       {nullopt, nullopt, nullopt}},
       {"match by tag",
        {by_tags({{"generation", "second"}})},
-       {x, x, x},
-       {8, 1.0, x},
-       {8, 1.0, x},
-       {x, x, x}},
+       {nullopt, nullopt, nullopt},
+       {8, 1.0, nullopt},
+       {8, 1.0, nullopt},
+       {nullopt, nullopt, nullopt}},
       {"match by name and tag",
        {by_name_and_tags("child", {{"generation", "second"}})},
-       {x, x, x},
-       {8, 1.0, x},
-       {x, x, x},
-       {x, x, x}},
+       {nullopt, nullopt, nullopt},
+       {8, 1.0, nullopt},
+       {nullopt, nullopt, nullopt},
+       {nullopt, nullopt, nullopt}},
   }));
 
   TracerConfig config;
@@ -221,8 +219,12 @@ TEST_CASE("span rules only on trace drop") {
   };
 
   auto test_case = GENERATE(values<TestCase>({
-      {"trace drop → span sampling tags", TestCase::DROP_TRACE, {8, 1.0, x}},
-      {"trace keep →  no span sampling tags", TestCase::KEEP_TRACE, {x, x, x}},
+      {"trace drop → span sampling tags",
+       TestCase::DROP_TRACE,
+       {8, 1.0, nullopt}},
+      {"trace keep →  no span sampling tags",
+       TestCase::KEEP_TRACE,
+       {nullopt, nullopt, nullopt}},
   }));
 
   CAPTURE(test_case.name);
@@ -256,8 +258,8 @@ TEST_CASE("span rule sample rate") {
   };
 
   auto test_case = GENERATE(values<TestCase>({
-      {"100% → span sampling tags", 1.0, {8, 1.0, x}},
-      {"0% →  no span sampling tags", 0.0, {x, x, x}},
+      {"100% → span sampling tags", 1.0, {8, 1.0, nullopt}},
+      {"0% →  no span sampling tags", 0.0, {nullopt, nullopt, nullopt}},
   }));
 
   CAPTURE(test_case.name);
@@ -297,7 +299,7 @@ TEST_CASE("span rule limiter") {
   };
 
   auto test_case =
-      GENERATE(values<TestCase>({{"default is no limit", 1000, x, 1000},
+      GENERATE(values<TestCase>({{"default is no limit", 1000, nullopt, 1000},
                                  {"limiter limits", 1000, 100, 100}}));
 
   CAPTURE(test_case.name);

diff --git a/test/test_trace_sampler.cpp b/test/test_trace_sampler.cpp
@@ -66,7 +66,7 @@ TEST_CASE("trace sampling rule sample rate") {
   config.service = "testsvc";
   config.trace_sampler.sample_rate = test_case.sample_rate;
   // Plenty of head room so that the limiter doesn't throttle us.
-  config.trace_sampler.max_per_second = num_iterations * 2;
+  config.trace_sampler.max_per_second = static_cast<double>(num_iterations * 2);
   const auto collector = std::make_shared<PriorityCountingCollector>();
   config.collector = collector;
   config.logger = std::make_shared<NullLogger>();
@@ -179,7 +179,7 @@ TEST_CASE("priority sampling") {
   config.service = "testsvc";
   config.environment = "dev";
   // plenty of head room
-  config.trace_sampler.max_per_second = 2 * num_iterations;
+  config.trace_sampler.max_per_second = static_cast<double>(2 * num_iterations);
   const auto collector =
       std::make_shared<PriorityCountingCollectorWithResponse>();
   config.collector = collector;