Skip to content
4 changes: 2 additions & 2 deletions .github/workflows/clang-tidy.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,9 @@ jobs:
matrix:
include:
- cmake_options: all-options-abiv1-preview
warning_limit: 215
warning_limit: 172
- cmake_options: all-options-abiv2-preview
warning_limit: 216
warning_limit: 173
steps:
- name: Harden the runner (Audit all outbound calls)
uses: step-security/harden-runner@002fdce3c6a235733a90a27c80493a3241e56863 # v2.12.1
Expand Down
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@ Increment the:
* [REMOVAL] Removed deprecated semantic convention header files
[#3475](https://github.com/open-telemetry/opentelemetry-cpp/pull/3475)

* [CodeHealth] Fix clang-tidy warnings part 1
[#3493](https://github.com/open-telemetry/opentelemetry-cpp/pull/3493)

Important changes:

* [REMOVAL] Removed deprecated semantic convention header files
Expand Down
8 changes: 4 additions & 4 deletions examples/otlp/grpc_metric_main.cc
Original file line number Diff line number Diff line change
Expand Up @@ -117,10 +117,10 @@ int main(int argc, char *argv[])
}
}
}
std::cout << "Using endpoint: " << exporter_options.endpoint << std::endl;
std::cout << "Using example type: " << example_type << std::endl;
std::cout << "Using cacert path: " << exporter_options.ssl_credentials_cacert_path << std::endl;
std::cout << "Using ssl credentials: " << exporter_options.use_ssl_credentials << std::endl;
std::cout << "Using endpoint: " << exporter_options.endpoint << "\n";
std::cout << "Using example type: " << example_type << "\n";
std::cout << "Using cacert path: " << exporter_options.ssl_credentials_cacert_path << "\n";
std::cout << "Using ssl credentials: " << exporter_options.use_ssl_credentials << "\n";

// Removing this line will leave the default noop MetricProvider in place.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,9 +81,9 @@ class OtlpFileClient
bool is_shutdown_;

// The configuration options associated with this file client.
const OtlpFileClientOptions options_;
OtlpFileClientOptions options_;
// The runtime options associated with this file client.
const OtlpFileClientRuntimeOptions runtime_options_;
OtlpFileClientRuntimeOptions runtime_options_;

opentelemetry::nostd::shared_ptr<OtlpFileAppender> backend_;
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,6 @@ namespace otlp
*/
struct OtlpFileClientRuntimeOptions
{
OtlpFileClientRuntimeOptions() = default;
~OtlpFileClientRuntimeOptions() = default;

std::shared_ptr<sdk::common::ThreadInstrumentation> thread_instrumentation =
std::shared_ptr<sdk::common::ThreadInstrumentation>(nullptr);
};
Expand Down
17 changes: 10 additions & 7 deletions exporters/otlp/src/otlp_file_client.cc
Original file line number Diff line number Diff line change
Expand Up @@ -719,11 +719,11 @@ static inline char HexEncode(unsigned char byte)
#endif
if (byte >= 10)
{
return byte - 10 + 'a';
return static_cast<char>(byte - 10 + 'a');
}
else
{
return byte + '0';
return static_cast<char>(byte + '0');
}
}

Expand Down Expand Up @@ -1004,6 +1004,11 @@ class OPENTELEMETRY_LOCAL_SYMBOL OtlpFileSystemBackend : public OtlpFileAppender
}
}

OtlpFileSystemBackend(const OtlpFileSystemBackend &) = delete;
OtlpFileSystemBackend &operator=(const OtlpFileSystemBackend &) = delete;
OtlpFileSystemBackend(OtlpFileSystemBackend &&) = delete;
OtlpFileSystemBackend &operator=(OtlpFileSystemBackend &&) = delete;

// Written size is not required to be precise, we can just ignore tsan report here.
OPENTELEMETRY_SANITIZER_NO_THREAD void MaybeRotateLog(std::size_t data_size)
{
Expand Down Expand Up @@ -1588,11 +1593,9 @@ class OPENTELEMETRY_LOCAL_SYMBOL OtlpFileOstreamBackend : public OtlpFileAppende
public:
explicit OtlpFileOstreamBackend(const std::reference_wrapper<std::ostream> &os) : os_(os) {}

~OtlpFileOstreamBackend() override {}
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixes

warning: class 'OtlpFileOstreamBackend' defines a non-default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]


void Export(nostd::string_view data, std::size_t /*record_count*/) override
{
os_.get().write(data.data(), data.size());
os_.get().write(data.data(), static_cast<std::streamsize>(data.size()));
}

bool ForceFlush(std::chrono::microseconds /*timeout*/) noexcept override
Expand All @@ -1611,8 +1614,8 @@ class OPENTELEMETRY_LOCAL_SYMBOL OtlpFileOstreamBackend : public OtlpFileAppende
OtlpFileClient::OtlpFileClient(OtlpFileClientOptions &&options,
OtlpFileClientRuntimeOptions &&runtime_options)
: is_shutdown_(false),
options_(std::move(options)),
runtime_options_(std::move(runtime_options))
options_{std::move(options)},
runtime_options_{std::move(runtime_options)}
{
if (nostd::holds_alternative<OtlpFileClientFileSystemOptions>(options_.backend_options))
{
Expand Down
20 changes: 10 additions & 10 deletions exporters/otlp/src/otlp_http_client.cc
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ class ResponseHandler : public http_client::EventHandler
error_message << "[OTLP HTTP Client] Session state: session create failed.";
if (!reason.empty())
{
error_message.write(reason.data(), reason.size());
error_message.write(reason.data(), static_cast<std::streamsize>(reason.size()));
}
OTEL_INTERNAL_LOG_ERROR(error_message.str());
}
Expand Down Expand Up @@ -207,7 +207,7 @@ class ResponseHandler : public http_client::EventHandler
error_message << "[OTLP HTTP Client] Session state: connection failed.";
if (!reason.empty())
{
error_message.write(reason.data(), reason.size());
error_message.write(reason.data(), static_cast<std::streamsize>(reason.size()));
}
OTEL_INTERNAL_LOG_ERROR(error_message.str());
}
Expand All @@ -232,7 +232,7 @@ class ResponseHandler : public http_client::EventHandler
error_message << "[OTLP HTTP Client] Session state: request send failed.";
if (!reason.empty())
{
error_message.write(reason.data(), reason.size());
error_message.write(reason.data(), static_cast<std::streamsize>(reason.size()));
}
OTEL_INTERNAL_LOG_ERROR(error_message.str());
}
Expand All @@ -250,7 +250,7 @@ class ResponseHandler : public http_client::EventHandler
error_message << "[OTLP HTTP Client] Session state: SSL handshake failed.";
if (!reason.empty())
{
error_message.write(reason.data(), reason.size());
error_message.write(reason.data(), static_cast<std::streamsize>(reason.size()));
}
OTEL_INTERNAL_LOG_ERROR(error_message.str());
}
Expand All @@ -261,7 +261,7 @@ class ResponseHandler : public http_client::EventHandler
error_message << "[OTLP HTTP Client] Session state: request time out.";
if (!reason.empty())
{
error_message.write(reason.data(), reason.size());
error_message.write(reason.data(), static_cast<std::streamsize>(reason.size()));
}
OTEL_INTERNAL_LOG_ERROR(error_message.str());
}
Expand All @@ -272,7 +272,7 @@ class ResponseHandler : public http_client::EventHandler
error_message << "[OTLP HTTP Client] Session state: network error.";
if (!reason.empty())
{
error_message.write(reason.data(), reason.size());
error_message.write(reason.data(), static_cast<std::streamsize>(reason.size()));
}
OTEL_INTERNAL_LOG_ERROR(error_message.str());
}
Expand All @@ -297,7 +297,7 @@ class ResponseHandler : public http_client::EventHandler
error_message << "[OTLP HTTP Client] Session state: (manually) cancelled.";
if (!reason.empty())
{
error_message.write(reason.data(), reason.size());
error_message.write(reason.data(), static_cast<std::streamsize>(reason.size()));
}
OTEL_INTERNAL_LOG_ERROR(error_message.str());
}
Expand Down Expand Up @@ -376,11 +376,11 @@ static inline char HexEncode(unsigned char byte)
#endif
if (byte >= 10)
{
return byte - 10 + 'a';
return static_cast<char>(byte - 10 + 'a');
}
else
{
return byte + '0';
return static_cast<char>(byte + '0');
}
}

Expand Down Expand Up @@ -664,7 +664,7 @@ void ConvertListFieldToJson(nlohmann::json &value,

OtlpHttpClient::OtlpHttpClient(OtlpHttpClientOptions &&options)
: is_shutdown_(false),
options_(options),
options_(std::move(options)),
http_client_(http_client::HttpClientFactory::Create(options.thread_instrumentation)),
start_session_counter_(0),
finished_session_counter_(0)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ class LongLastValueAggregation : public Aggregation
{
public:
LongLastValueAggregation();
LongLastValueAggregation(LastValuePointData &&);
LongLastValueAggregation(const LastValuePointData &);

void Aggregate(int64_t value, const PointAttributes &attributes = {}) noexcept override;
Expand All @@ -43,16 +42,15 @@ class DoubleLastValueAggregation : public Aggregation
{
public:
DoubleLastValueAggregation();
DoubleLastValueAggregation(LastValuePointData &&);
Copy link
Member Author

Choose a reason for hiding this comment

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

LastValuePointData is trivially-copyable and std::move has no effect.

DoubleLastValueAggregation(const LastValuePointData &);

void Aggregate(int64_t /* value */, const PointAttributes & /* attributes */) noexcept override {}

void Aggregate(double value, const PointAttributes &attributes = {}) noexcept override;

virtual std::unique_ptr<Aggregation> Merge(const Aggregation &delta) const noexcept override;
std::unique_ptr<Aggregation> Merge(const Aggregation &delta) const noexcept override;

virtual std::unique_ptr<Aggregation> Diff(const Aggregation &next) const noexcept override;
std::unique_ptr<Aggregation> Diff(const Aggregation &next) const noexcept override;

PointType ToPoint() const noexcept override;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ class LongSumAggregation : public Aggregation
{
public:
LongSumAggregation(bool is_monotonic);
LongSumAggregation(SumPointData &&);
Copy link
Member Author

Choose a reason for hiding this comment

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

SumPointData is trivially-copyable and std::move has no effect.

LongSumAggregation(const SumPointData &);

void Aggregate(int64_t value, const PointAttributes &attributes = {}) noexcept override;
Expand All @@ -44,7 +43,6 @@ class DoubleSumAggregation : public Aggregation
{
public:
DoubleSumAggregation(bool is_monotonic);
DoubleSumAggregation(SumPointData &&);
DoubleSumAggregation(const SumPointData &);

void Aggregate(int64_t /* value */, const PointAttributes & /* attributes */) noexcept override {}
Expand Down
2 changes: 1 addition & 1 deletion sdk/src/logs/batch_log_record_processor.cc
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ void BatchLogRecordProcessor::OnEmit(std::unique_ptr<Recordable> &&record) noexc
return;
}

if (buffer_.Add(std::unique_ptr<Recordable>(record.release())) == false)
if (buffer_.Add(std::move(record)) == false)
{
return;
}
Expand Down
9 changes: 6 additions & 3 deletions sdk/src/logs/multi_log_record_processor.cc
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,13 @@ namespace logs
MultiLogRecordProcessor::MultiLogRecordProcessor(
std::vector<std::unique_ptr<LogRecordProcessor>> &&processors)
{
for (auto &processor : processors)
auto log_record_processors = std::move(processors);
for (auto &processor : log_record_processors)
{
AddProcessor(std::move(processor));
}
}

MultiLogRecordProcessor::~MultiLogRecordProcessor()
{
ForceFlush();
Expand Down Expand Up @@ -54,11 +56,12 @@ std::unique_ptr<Recordable> MultiLogRecordProcessor::MakeRecordable() noexcept

void MultiLogRecordProcessor::OnEmit(std::unique_ptr<Recordable> &&record) noexcept
{
if (!record)
auto log_record = std::move(record);
if (!log_record)
{
return;
}
auto multi_recordable = static_cast<MultiRecordable *>(record.get());
auto multi_recordable = static_cast<MultiRecordable *>(log_record.get());

for (auto &processor : processors_)
{
Expand Down
3 changes: 2 additions & 1 deletion sdk/src/logs/simple_log_record_processor.cc
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,8 @@ std::unique_ptr<Recordable> SimpleLogRecordProcessor::MakeRecordable() noexcept
*/
void SimpleLogRecordProcessor::OnEmit(std::unique_ptr<Recordable> &&record) noexcept
{
nostd::span<std::unique_ptr<Recordable>> batch(&record, 1);
auto log_record = std::move(record);
nostd::span<std::unique_ptr<Recordable>> batch(&log_record, 1);
// Get lock to ensure Export() is never called concurrently
const std::lock_guard<opentelemetry::common::SpinLockMutex> locked(lock_);

Expand Down
23 changes: 8 additions & 15 deletions sdk/src/metrics/aggregation/lastvalue_aggregation.cc
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
#include <chrono>
#include <memory>
#include <mutex>
#include <utility>

#include "opentelemetry/common/spin_lock_mutex.h"
#include "opentelemetry/common/timestamp.h"
Expand All @@ -28,8 +27,6 @@ LongLastValueAggregation::LongLastValueAggregation()
point_data_.value_ = static_cast<int64_t>(0);
}

LongLastValueAggregation::LongLastValueAggregation(LastValuePointData &&data) : point_data_{data} {}

LongLastValueAggregation::LongLastValueAggregation(const LastValuePointData &data)
: point_data_{data}
{}
Expand All @@ -50,12 +47,12 @@ std::unique_ptr<Aggregation> LongLastValueAggregation::Merge(
nostd::get<LastValuePointData>(delta.ToPoint()).sample_ts_.time_since_epoch())
{
LastValuePointData merge_data = nostd::get<LastValuePointData>(ToPoint());
return std::unique_ptr<Aggregation>(new LongLastValueAggregation(std::move(merge_data)));
return std::unique_ptr<Aggregation>(new LongLastValueAggregation(merge_data));
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixes:

warning: std::move of the variable 'merge_data' of the trivially-copyable type 'LastValuePointData' has no effect [performance-move-const-arg]

Copy link
Member

Choose a reason for hiding this comment

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

I bellieve it's a mistake of clang. But it's OK to copy LastValuePointData because it's small and will not cost a lot.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the review. The class is trivially copyable, but we could consider disabling this warning with clang-tidy and keep the std::move. I'd prefer to keep the warning enabled and remove the moves that don't have any effect.

}
else
{
LastValuePointData merge_data = nostd::get<LastValuePointData>(delta.ToPoint());
return std::unique_ptr<Aggregation>(new LongLastValueAggregation(std::move(merge_data)));
return std::unique_ptr<Aggregation>(new LongLastValueAggregation(merge_data));
}
}

Expand All @@ -65,12 +62,12 @@ std::unique_ptr<Aggregation> LongLastValueAggregation::Diff(const Aggregation &n
nostd::get<LastValuePointData>(next.ToPoint()).sample_ts_.time_since_epoch())
{
LastValuePointData diff_data = nostd::get<LastValuePointData>(ToPoint());
return std::unique_ptr<Aggregation>(new LongLastValueAggregation(std::move(diff_data)));
return std::unique_ptr<Aggregation>(new LongLastValueAggregation(diff_data));
}
else
{
LastValuePointData diff_data = nostd::get<LastValuePointData>(next.ToPoint());
return std::unique_ptr<Aggregation>(new LongLastValueAggregation(std::move(diff_data)));
return std::unique_ptr<Aggregation>(new LongLastValueAggregation(diff_data));
}
}

Expand All @@ -86,10 +83,6 @@ DoubleLastValueAggregation::DoubleLastValueAggregation()
point_data_.value_ = 0.0;
}

DoubleLastValueAggregation::DoubleLastValueAggregation(LastValuePointData &&data)
: point_data_{data}
{}

DoubleLastValueAggregation::DoubleLastValueAggregation(const LastValuePointData &data)
: point_data_{data}
{}
Expand All @@ -110,12 +103,12 @@ std::unique_ptr<Aggregation> DoubleLastValueAggregation::Merge(
nostd::get<LastValuePointData>(delta.ToPoint()).sample_ts_.time_since_epoch())
{
LastValuePointData merge_data = nostd::get<LastValuePointData>(ToPoint());
return std::unique_ptr<Aggregation>(new DoubleLastValueAggregation(std::move(merge_data)));
return std::unique_ptr<Aggregation>(new DoubleLastValueAggregation(merge_data));
}
else
{
LastValuePointData merge_data = nostd::get<LastValuePointData>(delta.ToPoint());
return std::unique_ptr<Aggregation>(new DoubleLastValueAggregation(std::move(merge_data)));
return std::unique_ptr<Aggregation>(new DoubleLastValueAggregation(merge_data));
}
}

Expand All @@ -126,12 +119,12 @@ std::unique_ptr<Aggregation> DoubleLastValueAggregation::Diff(
nostd::get<LastValuePointData>(next.ToPoint()).sample_ts_.time_since_epoch())
{
LastValuePointData diff_data = nostd::get<LastValuePointData>(ToPoint());
return std::unique_ptr<Aggregation>(new DoubleLastValueAggregation(std::move(diff_data)));
return std::unique_ptr<Aggregation>(new DoubleLastValueAggregation(diff_data));
}
else
{
LastValuePointData diff_data = nostd::get<LastValuePointData>(next.ToPoint());
return std::unique_ptr<Aggregation>(new DoubleLastValueAggregation(std::move(diff_data)));
return std::unique_ptr<Aggregation>(new DoubleLastValueAggregation(diff_data));
}
}

Expand Down
4 changes: 0 additions & 4 deletions sdk/src/metrics/aggregation/sum_aggregation.cc
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,6 @@ LongSumAggregation::LongSumAggregation(bool is_monotonic)
point_data_.is_monotonic_ = is_monotonic;
}

LongSumAggregation::LongSumAggregation(SumPointData &&data) : point_data_{data} {}

LongSumAggregation::LongSumAggregation(const SumPointData &data) : point_data_{data} {}

void LongSumAggregation::Aggregate(int64_t value, const PointAttributes & /* attributes */) noexcept
Expand Down Expand Up @@ -81,8 +79,6 @@ DoubleSumAggregation::DoubleSumAggregation(bool is_monotonic)
point_data_.is_monotonic_ = is_monotonic;
}

DoubleSumAggregation::DoubleSumAggregation(SumPointData &&data) : point_data_(data) {}

DoubleSumAggregation::DoubleSumAggregation(const SumPointData &data) : point_data_(data) {}

void DoubleSumAggregation::Aggregate(double value,
Expand Down
Loading
Loading