[CodeHealth] Fix clang-tidy warnings part 1#3493
[CodeHealth] Fix clang-tidy warnings part 1#3493marcalff merged 11 commits intoopen-telemetry:mainfrom
Conversation
✅ Deploy Preview for opentelemetry-cpp-api-docs canceled.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3493 +/- ##
==========================================
+ Coverage 89.92% 89.95% +0.03%
==========================================
Files 219 219
Lines 7042 7039 -3
==========================================
- Hits 6332 6331 -1
+ Misses 710 708 -2
🚀 New features to boost your workflow:
|
dbarker
changed the title
| { | ||
| 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)); |
There was a problem hiding this comment.
Fixes:
warning: std::move of the variable 'merge_data' of the trivially-copyable type 'LastValuePointData' has no effect [performance-move-const-arg]
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| { | ||
| public: | ||
| DoubleLastValueAggregation(); | ||
| DoubleLastValueAggregation(LastValuePointData &&); |
There was a problem hiding this comment.
LastValuePointData is trivially-copyable and std::move has no effect.
| public: | ||
| explicit OtlpFileOstreamBackend(const std::reference_wrapper<std::ostream> &os) : os_(os) {} | ||
|
|
||
| ~OtlpFileOstreamBackend() override {} |
There was a problem hiding this comment.
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]
| { | ||
| public: | ||
| LongSumAggregation(bool is_monotonic); | ||
| LongSumAggregation(SumPointData &&); |
There was a problem hiding this comment.
SumPointData is trivially-copyable and std::move has no effect.
| { | ||
| 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)); |
There was a problem hiding this comment.
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.
marcalff
left a comment
marcalff
left a comment
There was a problem hiding this comment.
LGTM, thanks for the cleanup.
3 participants
Contributes to #2053
Changes
Fix some clang-tidy warnings:
For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes