Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Code health] Perform cppcheck cleanup #3150

Merged
merged 26 commits into from
Dec 6, 2024
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
9375819
First pass
chusitoo Nov 17, 2024
b89fbde
Second pass
chusitoo Nov 18, 2024
558560e
Add cppcheck in CI
chusitoo Nov 18, 2024
67ed142
Re-order initialization of member variables
chusitoo Nov 18, 2024
e8e1b85
Fix formatting
chusitoo Nov 18, 2024
d79e186
Put back some verbosity
chusitoo Nov 18, 2024
207c3e3
Fix unintialized buffer in copy ctor
chusitoo Nov 18, 2024
9305c6c
Revert ordering of members
chusitoo Nov 19, 2024
7e1e9ca
Merge remote-tracking branch 'remotes/origin/main' into cppcheck
chusitoo Nov 19, 2024
f706996
Merge remote-tracking branch 'remotes/origin/main' into cppcheck
chusitoo Nov 19, 2024
f1b18df
Fix reorder error in maintainer mode
chusitoo Nov 19, 2024
86ddede
Merge remote-tracking branch 'remotes/origin/main' into cppcheck
chusitoo Nov 19, 2024
e9b2252
Code review feedback + minor touchup
chusitoo Nov 20, 2024
6c062f8
Code review part 2
chusitoo Nov 22, 2024
48099c2
Merge remote-tracking branch 'remotes/origin/main' into cppcheck
chusitoo Nov 22, 2024
869f157
Merge remote-tracking branch 'remotes/origin/main' into cppcheck
chusitoo Nov 22, 2024
21e6d74
Merge remote-tracking branch 'remotes/origin/main' into cppcheck
chusitoo Nov 23, 2024
f22a295
Code review part 3
chusitoo Nov 23, 2024
4f2c120
Merge branch 'main' into cppcheck
marcalff Nov 25, 2024
d8151b3
Merge branch 'main' into cppcheck
marcalff Nov 25, 2024
4a75894
Merge branch 'main' into cppcheck
marcalff Nov 27, 2024
6c6925b
Fix iwyu warnings
chusitoo Nov 28, 2024
fabee6a
Fix format order of includes
chusitoo Nov 28, 2024
72fba18
Merge branch 'main' into cppcheck
marcalff Dec 2, 2024
3e01c95
Merge branch 'main' into cppcheck
chusitoo Dec 3, 2024
e745948
Merge branch 'main' into cppcheck
marcalff Dec 6, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 62 additions & 0 deletions .github/workflows/cppcheck.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@

name: cppcheck

on:
push:
branches: [ main ]
pull_request:
branches: [ main ]

jobs:
cppcheck:
runs-on: ubuntu-24.04
steps:
- uses: actions/checkout@v4
with:
submodules: 'recursive'

- name: Set up dependencies
run: |
sudo apt update -y
sudo apt install -y cppcheck

- name: Run cppcheck
run: |
cppcheck --version | tee cppcheck.log
cppcheck \
--force \
--enable=warning,performance,portability \
--inline-suppr \
--suppress=unknownMacro:exporters/etw/include/opentelemetry/exporters/etw/TraceLoggingDynamic.h \
--language=c++ \
--std=c++14 \
-I api/include \
-I exporters/elasticsearch/include \
-I exporters/etw/include \
-I exporters/memory/include \
-I exporters/ostream/include \
-I exporters/otlp/include \
-I exporters/prometheus/include \
-I exporters/zipkin/include \
-I ext/include \
-I opentracing-shim/include \
-I sdk/include \
-i build \
-i test \
-i third_party \
-j $(nproc) \
. 2>&1 | tee --append cppcheck.log

- uses: actions/upload-artifact@v4
if: success() || failure()
with:
name: Logs (cppcheck)
path: ./cppcheck.log

- name: Count warnings
run: |
set +e
COUNT=`grep -c -E "\[.+\]" cppcheck.log`
echo "cppcheck reported ${COUNT} warning(s)"
# TODO: uncomment to enforce failing the build
# if [ $COUNT -ne 0 ] ; then exit 1 ; fi
marcalff marked this conversation as resolved.
Show resolved Hide resolved
2 changes: 1 addition & 1 deletion api/include/opentelemetry/baggage/baggage_context.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
return context.SetValue(kBaggageHeader, baggage);
}
Expand Down
32 changes: 19 additions & 13 deletions api/include/opentelemetry/context/context.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,16 +29,14 @@ class Context
// hold a shared_ptr to the head of the DataList linked list
template <class T>
Context(const T &keys_and_values) noexcept
{
head_ = nostd::shared_ptr<DataList>{new DataList(keys_and_values)};
}
: head_{nostd::shared_ptr<DataList>{new DataList(keys_and_values)}}
{}
Comment on lines +32 to +33
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved as ABI safe, implementation change only.


// Creates a context object from a key and value, this will
// hold a shared_ptr to the head of the DataList linked list
Context(nostd::string_view key, ContextValue value) noexcept
{
head_ = nostd::shared_ptr<DataList>{new DataList(key, value)};
}
: head_{nostd::shared_ptr<DataList>{new DataList(key, value)}}
{}
Comment on lines +38 to +39
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved as ABI safe, implementation change only.


// Accepts a new iterable and then returns a new context that
// contains the new key and value data. It attaches the
Expand Down Expand Up @@ -92,22 +90,21 @@ class Context

private:
// A linked list to contain the keys and values of this context node
class DataList
struct DataList
{
public:
char *key_;
char *key_ = nullptr;

nostd::shared_ptr<DataList> next_;

size_t key_length_;
size_t key_length_ = 0UL;

ContextValue value_;

DataList() { next_ = nullptr; }
DataList() : next_{nullptr} {}

// Builds a data list off of a key and value iterable and returns the head
template <class T>
DataList(const T &keys_and_vals) : key_{nullptr}, next_(nostd::shared_ptr<DataList>{nullptr})
DataList(const T &keys_and_vals) : next_(nostd::shared_ptr<DataList>{nullptr})
{
bool first = true;
auto *node = this;
Expand All @@ -133,8 +130,17 @@ class Context
key_ = new char[key.size()];
key_length_ = key.size();
memcpy(key_, key.data(), key.size() * sizeof(char));
value_ = value;
next_ = nostd::shared_ptr<DataList>{nullptr};
value_ = value;
}

DataList(const DataList &other)
marcalff marked this conversation as resolved.
Show resolved Hide resolved
: key_(new char[other.key_length_]),
key_length_(other.key_length_),
next_(other.next_),
value_(other.value_)
{
memcpy(key_, other.key_, other.key_length_ * sizeof(char));
marcalff marked this conversation as resolved.
Show resolved Hide resolved
}

DataList &operator=(DataList &&other) noexcept
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Helper inlined.
Approved as ABI safe.

{
std::lock_guard<common::SpinLockMutex> guard(GetLock());
GetPropagator() = prop;
Expand Down
3 changes: 2 additions & 1 deletion api/include/opentelemetry/context/runtime_context.h
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,8 @@ class OPENTELEMETRY_EXPORT RuntimeContext
*
* @param storage a custom runtime context storage
*/
static void SetRuntimeContextStorage(nostd::shared_ptr<RuntimeContextStorage> storage) noexcept
static void SetRuntimeContextStorage(
const nostd::shared_ptr<RuntimeContextStorage> &storage) noexcept
Comment on lines +155 to +156
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Helper inlined.
Approved as ABI safe.

{
GetStorage() = storage;
}
Expand Down
4 changes: 2 additions & 2 deletions api/include/opentelemetry/logs/event_id.h
Original file line number Diff line number Diff line change
Expand Up @@ -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]}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ABI safe, internal implementation change.

{
name_ = nostd::unique_ptr<char[]>{new char[name.length() + 1]};
std::copy(name.begin(), name.end(), name_.get());
name_.get()[name.length()] = 0;
}
Expand Down
4 changes: 2 additions & 2 deletions api/include/opentelemetry/logs/provider.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Helper inlined.
Approved as ABI safe.

{
std::lock_guard<common::SpinLockMutex> guard(GetLock());
GetProvider() = tp;
Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Helper inlined.
Approved as ABI safe.

{
std::lock_guard<common::SpinLockMutex> guard(GetLock());
GetEventProvider() = tp;
Expand Down
2 changes: 1 addition & 1 deletion api/include/opentelemetry/metrics/provider.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Helper inlined.
Approved as ABI safe.

{
std::lock_guard<common::SpinLockMutex> guard(GetLock());
GetProvider() = tp;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,7 @@ template <class ReturnType, class FunctionObject, std::size_t... BoundIndices>
struct MakeVisitationMatrix<ReturnType, FunctionObject, index_sequence<>,
index_sequence<BoundIndices...>> {
using ResultType = ReturnType (*)(FunctionObject&&);
// cppcheck-suppress [duplInheritedMember]
static constexpr ResultType Run() {
return &call_with_indices<ReturnType, FunctionObject,
(BoundIndices - 1)...>;
Expand Down Expand Up @@ -722,6 +723,7 @@ struct VariantCoreAccess {
Self* self, Args&&... args) {
Destroy(*self);
using New = typename absl::OTABSL_OPTION_NAMESPACE_NAME::variant_alternative<NewIndex, Self>::type;
// cppcheck-suppress [legacyUninitvar]
New* const result = ::new (static_cast<void*>(&self->state_))
New(absl::OTABSL_OPTION_NAMESPACE_NAME::forward<Args>(args)...);
self->index_ = NewIndex;
Expand Down Expand Up @@ -1310,6 +1312,7 @@ class VariantStateBaseDestructorNontrivial : protected VariantStateBase<T...> {
VariantStateBaseDestructorNontrivial* self;
};

// cppcheck-suppress [duplInheritedMember]
void destroy() { VisitIndices<sizeof...(T)>::Run(Destroyer{this}, index_); }

~VariantStateBaseDestructorNontrivial() { destroy(); }
Expand Down
2 changes: 1 addition & 1 deletion api/include/opentelemetry/nostd/shared_ptr.h
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ class shared_ptr

struct alignas(kAlignment) PlacementBuffer
{
char data[kMaxSize];
char data[kMaxSize]{};
};

class shared_ptr_wrapper
Expand Down
2 changes: 1 addition & 1 deletion api/include/opentelemetry/plugin/tracer.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

: tracer_{std::move(tracer)}, span_{span}
{}

Expand Down
3 changes: 2 additions & 1 deletion api/include/opentelemetry/trace/context.h
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,8 @@ inline bool IsRootSpan(const context::Context &context) noexcept
}

// Set Span into explicit context
inline context::Context SetSpan(context::Context &context, nostd::shared_ptr<Span> span) noexcept
inline context::Context SetSpan(context::Context &context,
const nostd::shared_ptr<Span> &span) noexcept
Comment on lines +37 to +38
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Helper inlined.
Approved as ABI safe.

{
return context.SetValue(kSpanKey, span);
}
Expand Down
2 changes: 1 addition & 1 deletion api/include/opentelemetry/trace/default_span.h
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ class DefaultSpan : public Span

nostd::string_view ToString() const noexcept { return "DefaultSpan"; }

DefaultSpan(SpanContext span_context) noexcept : span_context_(span_context) {}
DefaultSpan(const SpanContext &span_context) noexcept : span_context_(span_context) {}
marcalff marked this conversation as resolved.
Show resolved Hide resolved

// movable and copiable
DefaultSpan(DefaultSpan &&spn) noexcept : Span(), span_context_(spn.GetContext()) {}
Expand Down
2 changes: 1 addition & 1 deletion api/include/opentelemetry/trace/provider.h
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ class OPENTELEMETRY_EXPORT Provider
/**
* Changes the singleton TracerProvider.
*/
static void SetTracerProvider(nostd::shared_ptr<TracerProvider> tp) noexcept
static void SetTracerProvider(const nostd::shared_ptr<TracerProvider> &tp) noexcept
{
std::lock_guard<common::SpinLockMutex> guard(GetLock());
GetProvider() = tp;
Expand Down
4 changes: 2 additions & 2 deletions api/include/opentelemetry/trace/span_context.h
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ class SpanContext final
SpanId span_id,
TraceFlags trace_flags,
bool is_remote,
nostd::shared_ptr<TraceState> trace_state = TraceState::GetDefault()) noexcept
const nostd::shared_ptr<TraceState> &trace_state = TraceState::GetDefault()) noexcept
marcalff marked this conversation as resolved.
Show resolved Hide resolved
: trace_id_(trace_id),
span_id_(span_id),
trace_flags_(trace_flags),
Expand All @@ -64,7 +64,7 @@ class SpanContext final
const trace::SpanId &span_id() const noexcept { return span_id_; }

// @returns the trace_state associated with this span_context
const nostd::shared_ptr<trace::TraceState> trace_state() const noexcept { return trace_state_; }
const nostd::shared_ptr<trace::TraceState> &trace_state() const noexcept { return trace_state_; }

/*
* @param that SpanContext for comparing.
Expand Down
4 changes: 2 additions & 2 deletions examples/grpc/tracer_common.h
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ class GrpcClientCarrier : public opentelemetry::context::propagation::TextMapCar
context_->AddMetadata(std::string(key), std::string(value));
}

ClientContext *context_;
ClientContext *context_ = nullptr;
};

class GrpcServerCarrier : public opentelemetry::context::propagation::TextMapCarrier
Expand All @@ -69,7 +69,7 @@ class GrpcServerCarrier : public opentelemetry::context::propagation::TextMapCar
// Not required for server
}

ServerContext *context_;
ServerContext *context_ = nullptr;
};

void InitTracer()
Expand Down
4 changes: 2 additions & 2 deletions examples/http/server.h
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,13 @@ class HttpServer : public HTTP_SERVER_NS::HttpRequestCallback
std::atomic<bool> is_running_{false};

public:
HttpServer(std::string server_name = "test_server", uint16_t port = 8800) : port_(port)
HttpServer(const std::string &server_name = "test_server", uint16_t port = 8800) : port_(port)
{
server_.setServerName(server_name);
server_.setKeepalive(false);
}

void AddHandler(std::string path, HTTP_SERVER_NS::HttpRequestCallback *request_handler)
void AddHandler(const std::string &path, HTTP_SERVER_NS::HttpRequestCallback *request_handler)
{
server_.addHandler(path, *request_handler);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,12 +52,12 @@ struct ElasticsearchExporterOptions
* from elasticsearch
* @param console_debug If true, print the status of the exporter methods in the console
*/
ElasticsearchExporterOptions(std::string host = "localhost",
int port = 9200,
std::string index = "logs",
int response_timeout = 30,
bool console_debug = false,
HttpHeaders http_headers = {})
ElasticsearchExporterOptions(const std::string &host = "localhost",
int port = 9200,
const std::string &index = "logs",
int response_timeout = 30,
bool console_debug = false,
const HttpHeaders &http_headers = {})
: host_{host},
port_{port},
index_{index},
Expand Down
5 changes: 2 additions & 3 deletions exporters/otlp/src/otlp_file_client.cc
Original file line number Diff line number Diff line change
Expand Up @@ -968,7 +968,6 @@ class OPENTELEMETRY_LOCAL_SYMBOL OtlpFileSystemBackend : public OtlpFileAppender
explicit OtlpFileSystemBackend(const OtlpFileClientFileSystemOptions &options)
: options_(options), is_initialized_{false}
{
file_ = std::make_shared<FileStats>();
file_->is_shutdown.store(false);
file_->rotate_index = 0;
file_->written_size = 0;
Expand Down Expand Up @@ -1536,9 +1535,9 @@ class OPENTELEMETRY_LOCAL_SYMBOL OtlpFileSystemBackend : public OtlpFileAppender
std::mutex background_thread_waiter_lock;
std::condition_variable background_thread_waiter_cv;
};
std::shared_ptr<FileStats> file_;
std::shared_ptr<FileStats> file_ = std::make_shared<FileStats>();

std::atomic<bool> is_initialized_;
std::atomic<bool> is_initialized_{false};
std::time_t check_file_path_interval_{0};
};

Expand Down
Loading
Loading