diff --git a/.clang-tidy b/.clang-tidy index 599a02fd88..4457484e8d 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -5,6 +5,7 @@ Checks: > -*, performance-*, portability-*, + -portability-template-virtual-member-function, abseil-*, -abseil-string-find-str-contains, bugprone-*, diff --git a/.github/workflows/clang-tidy.yaml b/.github/workflows/clang-tidy.yaml index 2e977ec4bc..e9f25d4200 100644 --- a/.github/workflows/clang-tidy.yaml +++ b/.github/workflows/clang-tidy.yaml @@ -17,9 +17,9 @@ jobs: matrix: include: - cmake_options: all-options-abiv1-preview - warning_limit: 204 + warning_limit: 170 - cmake_options: all-options-abiv2-preview - warning_limit: 206 + warning_limit: 166 env: CC: /usr/bin/clang-18 CXX: /usr/bin/clang++-18 diff --git a/CHANGELOG.md b/CHANGELOG.md index c3dcd519e2..2426db82b0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,9 @@ Increment the: ## [Unreleased] +* [CODE HEALTH] Fix clang tidy warnings in API `common` and `context` + [#3948](https://github.com/open-telemetry/opentelemetry-cpp/pull/3948) + ## [1.26.0] 2026-03-19 * [RELEASE] Bump main branch to 1.26.0-dev diff --git a/api/include/opentelemetry/common/key_value_iterable.h b/api/include/opentelemetry/common/key_value_iterable.h index 4d191b07b7..e70a58dbb6 100644 --- a/api/include/opentelemetry/common/key_value_iterable.h +++ b/api/include/opentelemetry/common/key_value_iterable.h @@ -17,7 +17,12 @@ namespace common class KeyValueIterable { public: - virtual ~KeyValueIterable() = default; + KeyValueIterable() = default; + KeyValueIterable(const KeyValueIterable &) = default; + KeyValueIterable &operator=(const KeyValueIterable &) = default; + KeyValueIterable(KeyValueIterable &&) noexcept = default; + KeyValueIterable &operator=(KeyValueIterable &&) noexcept = default; + virtual ~KeyValueIterable() = default; /** * Iterate over key-value pairs @@ -40,8 +45,6 @@ class KeyValueIterable class NoopKeyValueIterable : public KeyValueIterable { public: - ~NoopKeyValueIterable() override = default; - /** * No-op implementation: does not invoke the callback, even if key-value pairs are present. * @return true without iterating or invoking the callback diff --git a/api/include/opentelemetry/common/key_value_iterable_view.h b/api/include/opentelemetry/common/key_value_iterable_view.h index a8fe310321..750d4207d0 100644 --- a/api/include/opentelemetry/common/key_value_iterable_view.h +++ b/api/include/opentelemetry/common/key_value_iterable_view.h @@ -122,7 +122,7 @@ MakeAttributes( * @return common::KeyValueIterable */ inline static const common::KeyValueIterable &MakeAttributes( - const common::KeyValueIterable &attributes) noexcept + const common::KeyValueIterable &attributes OPENTELEMETRY_ATTRIBUTE_LIFETIME_BOUND) noexcept { return attributes; } diff --git a/api/include/opentelemetry/common/kv_properties.h b/api/include/opentelemetry/common/kv_properties.h index b5accab403..edf3a83601 100644 --- a/api/include/opentelemetry/common/kv_properties.h +++ b/api/include/opentelemetry/common/kv_properties.h @@ -33,7 +33,7 @@ class KeyValueStringTokenizer KeyValueStringTokenizer( nostd::string_view str, const KeyValueStringTokenizerOptions &opts = KeyValueStringTokenizerOptions()) noexcept - : str_(str), opts_(opts), index_(0) + : str_(str), opts_(opts) {} static nostd::string_view GetDefaultKeyOrValue() @@ -129,7 +129,7 @@ class KeyValueStringTokenizer private: nostd::string_view str_; KeyValueStringTokenizerOptions opts_; - size_t index_; + size_t index_{}; }; // Class to store fixed size array of key-value pairs of string type @@ -140,7 +140,8 @@ class KeyValueProperties class Entry { public: - Entry() : key_(nullptr), value_(nullptr) {} + Entry() = default; + ~Entry() = default; // Copy constructor Entry(const Entry ©) @@ -150,16 +151,20 @@ class KeyValueProperties } // Copy assignment operator - Entry &operator=(Entry &other) + Entry &operator=(const Entry &other) { + if (this == &other) + { + return *this; + } key_ = CopyStringToPointer(other.key_.get()); value_ = CopyStringToPointer(other.value_.get()); return *this; } // Move contructor and assignment operator - Entry(Entry &&other) = default; - Entry &operator=(Entry &&other) = default; + Entry(Entry &&other) noexcept = default; + Entry &operator=(Entry &&other) noexcept = default; // Creates an Entry for a given key-value pair. Entry(nostd::string_view key, nostd::string_view value) @@ -194,10 +199,10 @@ class KeyValueProperties }; // Maintain the number of entries in entries_. - size_t num_entries_; + size_t num_entries_{}; // Max size of allocated array - size_t max_num_entries_; + size_t max_num_entries_{}; // Store entries in a C-style array to avoid using std::array or std::vector. nostd::unique_ptr entries_; @@ -205,18 +210,14 @@ class KeyValueProperties public: // Create Key-value list of given size // @param size : Size of list. - KeyValueProperties(size_t size) noexcept - : num_entries_(0), max_num_entries_(size), entries_(new Entry[size]) - {} + KeyValueProperties(size_t size) noexcept : max_num_entries_(size), entries_(new Entry[size]) {} // Create Empty Key-Value list - KeyValueProperties() noexcept : num_entries_(0), max_num_entries_(0), entries_(nullptr) {} + KeyValueProperties() noexcept = default; template ::value>::type> KeyValueProperties(const T &keys_and_values) noexcept - : num_entries_(0), - max_num_entries_(keys_and_values.size()), - entries_(new Entry[max_num_entries_]) + : max_num_entries_(keys_and_values.size()), entries_(new Entry[max_num_entries_]) { for (auto &e : keys_and_values) { diff --git a/api/include/opentelemetry/common/spin_lock_mutex.h b/api/include/opentelemetry/common/spin_lock_mutex.h index 7031fa4d23..24141867c7 100644 --- a/api/include/opentelemetry/common/spin_lock_mutex.h +++ b/api/include/opentelemetry/common/spin_lock_mutex.h @@ -56,6 +56,8 @@ class SpinLockMutex ~SpinLockMutex() noexcept = default; SpinLockMutex(const SpinLockMutex &) = delete; SpinLockMutex &operator=(const SpinLockMutex &) = delete; + SpinLockMutex(SpinLockMutex &&) = delete; + SpinLockMutex &operator=(SpinLockMutex &&) = delete; static inline void fast_yield() noexcept { diff --git a/api/include/opentelemetry/context/context.h b/api/include/opentelemetry/context/context.h index b279d53f84..507c1fb20a 100644 --- a/api/include/opentelemetry/context/context.h +++ b/api/include/opentelemetry/context/context.h @@ -44,7 +44,7 @@ class Context template Context SetValues(T &values) noexcept { - Context context = Context(values); + Context context(values); nostd::shared_ptr last = context.head_; while (last->next_ != nullptr) { @@ -59,7 +59,7 @@ class Context // exisiting list to the end of the new list. Context SetValue(nostd::string_view key, ContextValue value) noexcept { - Context context = Context(key, std::move(value)); + Context context(key, std::move(value)); context.head_->next_ = head_; return context; } @@ -92,11 +92,11 @@ class Context // A linked list to contain the keys and values of this context node struct DataList { - char *key_ = nullptr; + char *key_{nullptr}; nostd::shared_ptr next_{nullptr}; - size_t key_length_ = 0UL; + size_t key_length_{}; ContextValue value_; @@ -108,7 +108,7 @@ class Context { bool first = true; auto *node = this; - for (auto &iter : keys_and_vals) + for (const auto &iter : keys_and_vals) { if (first) { @@ -126,29 +126,28 @@ class Context // Builds a data list with just a key and value, so it will just be the head // and returns that head. DataList(nostd::string_view key, ContextValue value) + : key_{new char[key.size()]}, + next_{nostd::shared_ptr{nullptr}}, + key_length_{key.size()}, + value_{std::move(value)} { - key_ = new char[key.size()]; - key_length_ = key.size(); std::memcpy(key_, key.data(), key.size() * sizeof(char)); - next_ = nostd::shared_ptr{nullptr}; - value_ = std::move(value); } - DataList(const DataList &other) - : key_(new char[other.key_length_]), - next_(other.next_), - key_length_(other.key_length_), - value_(other.value_) - { - std::memcpy(key_, other.key_, other.key_length_ * sizeof(char)); - } + // Delete unused special member functions + DataList(const DataList &other) = delete; + DataList(DataList &&other) noexcept = delete; + DataList &operator=(const DataList &other) = delete; + // Move assignment operator is only called by DataList(const T &keys_and_vals) + // The moved to object has default member values DataList &operator=(DataList &&other) noexcept { key_length_ = other.key_length_; value_ = std::move(other.value_); next_ = std::move(other.next_); + // key_ has a default nullptr value and does not need to be deleted before this assignment key_ = other.key_; other.key_ = nullptr; diff --git a/api/include/opentelemetry/context/propagation/text_map_propagator.h b/api/include/opentelemetry/context/propagation/text_map_propagator.h index 21bc293616..2254dcc2dd 100644 --- a/api/include/opentelemetry/context/propagation/text_map_propagator.h +++ b/api/include/opentelemetry/context/propagation/text_map_propagator.h @@ -18,6 +18,12 @@ namespace propagation class TextMapCarrier { public: + TextMapCarrier() = default; + TextMapCarrier(const TextMapCarrier &) = default; + TextMapCarrier &operator=(const TextMapCarrier &) = default; + TextMapCarrier(TextMapCarrier &&) noexcept = default; + TextMapCarrier &operator=(TextMapCarrier &&) noexcept = default; + // returns the value associated with the passed key. virtual nostd::string_view Get(nostd::string_view key) const noexcept = 0; @@ -42,6 +48,12 @@ class TextMapCarrier class TextMapPropagator { public: + TextMapPropagator() = default; + TextMapPropagator(const TextMapPropagator &) = default; + TextMapPropagator &operator=(const TextMapPropagator &) = default; + TextMapPropagator(TextMapPropagator &&) noexcept = default; + TextMapPropagator &operator=(TextMapPropagator &&) noexcept = default; + // Returns the context that is stored in the carrier with the TextMapCarrier as extractor. virtual context::Context Extract(const TextMapCarrier &carrier, context::Context &context) noexcept = 0; diff --git a/api/include/opentelemetry/context/runtime_context.h b/api/include/opentelemetry/context/runtime_context.h index 0ef170571b..0d52a822f2 100644 --- a/api/include/opentelemetry/context/runtime_context.h +++ b/api/include/opentelemetry/context/runtime_context.h @@ -24,6 +24,11 @@ namespace context class Token { public: + Token(const Token &) = delete; + Token &operator=(const Token &) = delete; + Token(Token &&) = delete; + Token &operator=(Token &&) = delete; + bool operator==(const Context &other) const noexcept { return context_ == other; } virtual ~Token() noexcept; @@ -35,6 +40,8 @@ class Token // one that was passed in. Token(const Context &context) : context_(context) {} + // Keep the context_ member as const to ensure the Token is immutable. + // NOLINTNEXTLINE(cppcoreguidelines-avoid-const-or-ref-data-members) const Context context_; }; @@ -48,6 +55,13 @@ class Token class OPENTELEMETRY_EXPORT RuntimeContextStorage { public: + RuntimeContextStorage() = default; + // Keep RuntimeContextStorage copyable but not movable to preserve the existing API. + RuntimeContextStorage(const RuntimeContextStorage &) = default; + RuntimeContextStorage &operator=(const RuntimeContextStorage &) = default; + RuntimeContextStorage(RuntimeContextStorage &&) noexcept = delete; + RuntimeContextStorage &operator=(RuntimeContextStorage &&) noexcept = delete; + /** * Return the current context. * @return the current context @@ -68,7 +82,7 @@ class OPENTELEMETRY_EXPORT RuntimeContextStorage */ virtual bool Detach(Token &token) noexcept = 0; - virtual ~RuntimeContextStorage() {} + virtual ~RuntimeContextStorage() = default; protected: nostd::unique_ptr CreateToken(const Context &context) noexcept @@ -261,7 +275,11 @@ class ThreadLocalContextStorage : public RuntimeContextStorage */ static constexpr size_t max_capacity_ = 1000000; - Stack() noexcept : size_(0), capacity_(0), base_(nullptr) {} + Stack() noexcept = default; + Stack(const Stack &) = delete; + Stack &operator=(const Stack &) = delete; + Stack(Stack &&) = delete; + Stack &operator=(Stack &&) = delete; // Pops the top Context off the stack. void Pop() noexcept @@ -359,14 +377,14 @@ class ThreadLocalContextStorage : public RuntimeContextStorage ~Stack() noexcept { delete[] base_; } - size_t size_; - size_t capacity_; - Context *base_; + size_t size_{}; + size_t capacity_{}; + Context *base_{nullptr}; }; OPENTELEMETRY_API_SINGLETON Stack &GetStack() { - static thread_local Stack stack_ = Stack(); + static thread_local Stack stack_{}; return stack_; } }; diff --git a/api/test/common/kv_properties_test.cc b/api/test/common/kv_properties_test.cc index f85e8f38c7..4fb4bc15c9 100644 --- a/api/test/common/kv_properties_test.cc +++ b/api/test/common/kv_properties_test.cc @@ -49,6 +49,15 @@ TEST(EntryTest, Assignment) EXPECT_EQ(empty.GetValue(), e.GetValue()); } +TEST(EntryTest, SelfAssignment) +{ + KeyValueProperties::Entry e("test_key", "test_value"); + KeyValueProperties::Entry &self = e; + e = self; + EXPECT_EQ(e.GetKey(), "test_key"); + EXPECT_EQ(e.GetValue(), "test_value"); +} + TEST(EntryTest, SetValue) { KeyValueProperties::Entry e("test_key", "test_value"); diff --git a/ci/do_ci.sh b/ci/do_ci.sh index 21736234fd..e0e7c41949 100755 --- a/ci/do_ci.sh +++ b/ci/do_ci.sh @@ -3,7 +3,7 @@ # Copyright The OpenTelemetry Authors # SPDX-License-Identifier: Apache-2.0 -set -e +set -eo pipefail function install_prometheus_cpp_client {