[Code health] Perform cppcheck cleanup #3150

chusitoo · 2024-11-18T05:34:56Z

Changes

Cleaned up code based on current settings for CI to pass
Some warnings are either false positives and/or fail in v2.13 but no longer reported in v2.14+, although this would require building cppcheck from source
Also suppressed exporters/etw/include/opentelemetry/exporters/etw/TraceLoggingDynamic.h as it has dependencies on Windows macros (partially covered by microsoft_sal.cfg but not entirely). An alternative to suppressing it, maybe as a future improvement, could be to supply a custom cfg file with rules for cppcheck to ingest.

For significant contributions please make sure you have completed the following items:

CHANGELOG.md updated for non-trivial changes
Unit tests have been added
Changes in public API reviewed

netlify · 2024-11-18T05:35:09Z

✅ Deploy Preview for opentelemetry-cpp-api-docs canceled.

Name	Link
🔨 Latest commit	`48099c2`
🔍 Latest deploy log	https://app.netlify.com/sites/opentelemetry-cpp-api-docs/deploys/673ffff50a2a120008257a79

codecov · 2024-11-18T05:47:33Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.85%. Comparing base (fe68d51) to head (48099c2).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3150      +/-   ##
==========================================
- Coverage   87.86%   87.85%   -0.01%     
==========================================
  Files         195      195              
  Lines        6151     6153       +2     
==========================================
+ Hits         5404     5405       +1     
- Misses        747      748       +1

Files with missing lines	Coverage Δ
...pi/include/opentelemetry/baggage/baggage_context.h	`100.00% <ø> (ø)`
api/include/opentelemetry/context/context.h	`100.00% <100.00%> (ø)`
...ntelemetry/context/propagation/global_propagator.h	`100.00% <100.00%> (ø)`
...pi/include/opentelemetry/context/runtime_context.h	`97.60% <ø> (ø)`
api/include/opentelemetry/logs/event_id.h	`83.34% <100.00%> (ø)`
api/include/opentelemetry/logs/provider.h	`100.00% <100.00%> (ø)`
api/include/opentelemetry/metrics/provider.h	`100.00% <100.00%> (ø)`
...metry/nostd/internal/absl/types/internal/variant.h	`74.57% <ø> (ø)`
api/include/opentelemetry/nostd/shared_ptr.h	`100.00% <ø> (ø)`
api/include/opentelemetry/trace/context.h	`91.67% <100.00%> (ø)`
... and 9 more

... and 1 file with indirect coverage changes

---- 🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests

.github/workflows/cppcheck.yml

marcalff

LGTM, thanks for the contribution.

Please add a separate PR for the CI alone, which can be merged right away in the main branch. Comment out the last step, to not fail the build on errors, this will be enforced when cleanup is completed.

This will allow us reviewers to actually see the current cppcheck errors in the log, which will facilitate review, as we won't have to ask "why ?" or "what was the error ?" on various lines fixed.

After CI is merged, this PR will contain only the cleanup fixes then.

Comments on actual code cleanup to follow.

marcalff · 2024-11-18T23:09:30Z

api/include/opentelemetry/baggage/baggage_context.h

@@ -27,7 +27,7 @@ inline nostd::shared_ptr<Baggage> GetBaggage(const context::Context &context) no
 }

 inline context::Context SetBaggage(context::Context &context,
-                                   nostd::shared_ptr<Baggage> baggage) noexcept
+                                   const nostd::shared_ptr<Baggage> &baggage) noexcept


Providing details because this is not trivial, as justification, for all reviewers:

ABI change (the prototype is different)

on a helper API

which is inlined anyway, expanded in the caller compile unit

Hence:

there is no ABI change overall, as no API implemented by the SDK is touched

Approved, this is safe IMO.

marcalff · 2024-11-18T23:10:30Z

api/include/opentelemetry/context/context.h

+      : head_{nostd::shared_ptr<DataList>{new DataList(keys_and_values)}}
+  {}


Approved as ABI safe, implementation change only.

marcalff · 2024-11-18T23:10:40Z

api/include/opentelemetry/context/context.h

+      : head_{nostd::shared_ptr<DataList>{new DataList(key, value)}}
+  {}


Approved as ABI safe, implementation change only.

api/include/opentelemetry/context/context.h

marcalff · 2024-11-18T23:35:32Z

api/include/opentelemetry/context/context.h

+      value_ = value;
+    }
+
+    DataList(const DataList &other)


Ok to have a copy constructor, this clears the cppcheck warning, and is safer in case of refactoring.

I suspect this code to be unreachable.

marcalff · 2024-11-18T23:37:30Z

api/include/opentelemetry/context/propagation/global_propagator.h

@@ -32,7 +32,7 @@ class OPENTELEMETRY_EXPORT GlobalTextMapPropagator
    return nostd::shared_ptr<TextMapPropagator>(GetPropagator());
  }

-  static void SetGlobalPropagator(nostd::shared_ptr<TextMapPropagator> prop) noexcept
+  static void SetGlobalPropagator(const nostd::shared_ptr<TextMapPropagator> &prop) noexcept


Helper inlined.
Approved as ABI safe.

marcalff · 2024-11-18T23:37:40Z

api/include/opentelemetry/context/runtime_context.h

+  static void SetRuntimeContextStorage(
+      const nostd::shared_ptr<RuntimeContextStorage> &storage) noexcept


Helper inlined.
Approved as ABI safe.

marcalff · 2024-11-18T23:38:18Z

api/include/opentelemetry/logs/event_id.h

@@ -19,9 +19,9 @@ namespace logs
 class EventId
 {
 public:
-  EventId(int64_t id, nostd::string_view name) noexcept : id_{id}
+  EventId(int64_t id, nostd::string_view name) noexcept
+      : id_{id}, name_{nostd::unique_ptr<char[]>{new char[name.length() + 1]}}


ABI safe, internal implementation change.

marcalff · 2024-11-18T23:39:26Z

api/include/opentelemetry/logs/provider.h

@@ -39,7 +39,7 @@ class OPENTELEMETRY_EXPORT Provider
  /**
   * Changes the singleton LoggerProvider.
   */
-  static void SetLoggerProvider(nostd::shared_ptr<LoggerProvider> tp) noexcept
+  static void SetLoggerProvider(const nostd::shared_ptr<LoggerProvider> &tp) noexcept


Helper inlined.
Approved as ABI safe.

marcalff · 2024-11-18T23:39:40Z

api/include/opentelemetry/logs/provider.h

@@ -60,7 +60,7 @@ class OPENTELEMETRY_EXPORT Provider
  /**
   * Changes the singleton EventLoggerProvider.
   */
-  static void SetEventLoggerProvider(nostd::shared_ptr<EventLoggerProvider> tp) noexcept
+  static void SetEventLoggerProvider(const nostd::shared_ptr<EventLoggerProvider> &tp) noexcept


Helper inlined.
Approved as ABI safe.

marcalff · 2024-11-18T23:39:48Z

api/include/opentelemetry/metrics/provider.h

@@ -38,7 +38,7 @@ class Provider
  /**
   * Changes the singleton MeterProvider.
   */
-  static void SetMeterProvider(nostd::shared_ptr<MeterProvider> tp) noexcept
+  static void SetMeterProvider(const nostd::shared_ptr<MeterProvider> &tp) noexcept


Helper inlined.
Approved as ABI safe.

marcalff · 2024-11-18T23:44:24Z

api/include/opentelemetry/plugin/tracer.h

@@ -20,7 +20,7 @@ class DynamicLibraryHandle;
 class Span final : public trace::Span
 {
 public:
-  Span(std::shared_ptr<trace::Tracer> &&tracer, nostd::shared_ptr<trace::Span> span) noexcept
+  Span(std::shared_ptr<trace::Tracer> &&tracer, const nostd::shared_ptr<trace::Span> &span) noexcept


Here changing the span constructor does not change the object layout or virtual table, should be ok.

This plugin class in particular is not inherited from in the SDK, changing the constructor is ok.

Approved as ABI safe.

Beside, plugin/tracer.h is not used as far as I know.

api/include/opentelemetry/trace/default_span.h

marcalff · 2024-11-18T23:50:15Z

api/include/opentelemetry/trace/context.h

+inline context::Context SetSpan(context::Context &context,
+                                const nostd::shared_ptr<Span> &span) noexcept


Helper inlined.
Approved as ABI safe.

api/include/opentelemetry/trace/span_context.h

marcalff · 2024-11-19T01:03:13Z

@chusitoo The CI is now merged to main, please merge main into this PR to resolve the conflicts.

chusitoo · 2024-11-20T00:59:12Z

.github/workflows/cppcheck.yml

@@ -73,4 +58,5 @@ jobs:
          set +e
          COUNT=`grep -c -E "\[.+\]" cppcheck.log`
          echo "cppcheck reported ${COUNT} warning(s)"
-          if [ $COUNT -ne 0 ] ; then exit 1 ; fi
+          # TODO: uncomment to enforce failing the build
+          # if [ $COUNT -ne 0 ] ; then exit 1 ; fi


Should this check be re-enabled now?

Yes, please do.

If there are unresolved warnings left, it is ok to test with some margin, say count > 5.

The point is to prevent new errors to come in with more merges, while giving us time to resolve possibly more complex issues in follow up PRs and ultimately have count > 0.

Made the WARNING_LIMIT = 10 to match your PR for iwyu part 5 and to accommodate the 9 false positives that are no longer suppressed. I also thought it would be a good idea to make it stand out a bit more when the count is below threshold but not zero, by emitting a warning annotation.

marcalff · 2024-11-21T21:02:44Z

exporters/otlp/src/otlp_http_client.cc


-      return opentelemetry::sdk::common::ExportResult::kFailure;
+      const auto result = opentelemetry::sdk::common::ExportResult::kFailure;
+      result_callback(result);


Here and in other places, the call to result_callback() has moved a bit.

Could you provide more details ?

Absolutely. The scan reports an [accessMoved] warning here

This is due to the move that happens higher in OtlpHttpClient::Export() on L753 which seems as though it is likely undefined behavior calling it from that point on.

Since the moved from result_callback appears to be used only to consume the result from createSession(), my solution was to move the invocation closer to the call sites where it's been moved to, just before the function returns the actual result.

marcalff · 2024-11-21T21:05:46Z

ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h

@@ -309,6 +307,7 @@ class HttpClient : public opentelemetry::ext::http::client::HttpClient
  std::shared_ptr<opentelemetry::ext::http::client::Session> CreateSession(
      nostd::string_view url) noexcept override;

+  // cppcheck-suppress [virtualCallInConstructor]


Please remove the suppression.

Instead, if this still raises a warning in cppcheck, we need to find where from, and fix the caller code.

Please note that this and all other suppressions of [virtualCallInConstructor] are false positives no longer reported in v2.14+, as alluded to in the changelog summary.

I will revert, as requested. As a future improvement, using a more updated version of cppcheck (built from source) should mitigate these warnings.

@marcalff I am noticing that the changes I had put in circular_buffer.h to suppress both [arithOperationsOnVoidPointer] are no longer reported in v2.16, so I am wondering if all these false positives waiting to be addressed should be documented somewhere. I think this would be beneficial to avoid repeat investigation when the fix is updating the scanner version.

marcalff · 2024-11-21T21:10:06Z

sdk/include/opentelemetry/sdk/logs/batch_log_record_processor.h

@@ -85,6 +85,7 @@ class BatchLogRecordProcessor : public LogRecordProcessor
   *
   * NOTE: Timeout functionality not supported yet.
   */
+  // cppcheck-suppress [virtualCallInConstructor]
