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.

Sign up for GitHub

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

Visual Studio compiler warnings clean-up #315

Merged
merged 19 commits into from
Sep 9, 2020
Merged

Visual Studio compiler warnings clean-up #315

Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
fb2c087
Add json.hpp to CI and Build Tools
maxgolov Aug 28, 2020
4b7f120
Fix an issue with Ubuntu 18.04 not liking --ignore-missing
maxgolov Aug 28, 2020
6a8f1e4
Package name for json.hpp is different between Ubuntu 18.xx and 20.xx…
maxgolov Aug 28, 2020
a4d33da
Install either v2 or v3 of nlohmann json.hpp depending on distro
maxgolov Aug 28, 2020
d11c003
Based off latest opent-telemetry:master
maxgolov Sep 3, 2020
b667c84
Compiler warnings clean-up
maxgolov Sep 3, 2020
51890cf
Merge branch 'master' of https://github.com/open-telemetry/openteleme…
maxgolov Sep 3, 2020
806296f
Remove extra #endif
maxgolov Sep 3, 2020
d94bd2f
clang format
maxgolov Sep 3, 2020
aad6e5e
Format
maxgolov Sep 3, 2020
069ab1d
Format
maxgolov Sep 3, 2020
17eb8cc
Format
maxgolov Sep 3, 2020
9ba6d62
Format
maxgolov Sep 3, 2020
6a132ef
Format
maxgolov Sep 3, 2020
5b594b0
Format
maxgolov Sep 3, 2020
49098c7
Format
maxgolov Sep 3, 2020
d8eb8d9
Format
maxgolov Sep 3, 2020
ab2611c
One more try to format :D
maxgolov Sep 3, 2020
fc805cd
Addressing code review comments
maxgolov Sep 4, 2020
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
5 changes: 3 additions & 2 deletions api/include/opentelemetry/context/context.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#pragma once

#include <cstring>
#include "opentelemetry/context/context_value.h"
#include "opentelemetry/nostd/shared_ptr.h"
#include "opentelemetry/nostd/string_view.h"
Expand Down Expand Up @@ -65,7 +66,7 @@ class Context
{
if (key.size() == data->key_length_)
{
if (memcmp(key.data(), data->key_, data->key_length_) == 0)
if (std::memcmp(key.data(), data->key_, data->key_length_) == 0)
{
return data->value_;
}
Expand All @@ -81,7 +82,7 @@ class Context
{
if (key.size() == data->key_length_)
{
if (memcmp(key.data(), data->key_, data->key_length_) == 0)
if (std::memcmp(key.data(), data->key_, data->key_length_) == 0)
{
return true;
}
Expand Down
20 changes: 20 additions & 0 deletions api/include/opentelemetry/nostd/string_view.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,12 @@ class string_view
inline bool operator==(string_view lhs, string_view rhs) noexcept
{
return lhs.length() == rhs.length() &&
#if _MSC_VER == 1900
// Avoid SCL error in Visual Studio 2015
(std::memcmp(lhs.data(), rhs.data(), lhs.length()) == 0);
#else
std::equal(lhs.data(), lhs.data() + lhs.length(), rhs.data());
#endif
}

inline bool operator==(string_view lhs, const std::string &rhs) noexcept
Expand Down Expand Up @@ -172,3 +177,18 @@ inline std::ostream &operator<<(std::ostream &os, string_view s)
}
} // namespace nostd
OPENTELEMETRY_END_NAMESPACE

namespace std
{
template <>
struct hash<OPENTELEMETRY_NAMESPACE::nostd::string_view>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary for VS2015?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, sorry, this is a template that allows to use nostd::string_view as key in the std::map<nostd::string_view, ...> .. It's adding the missing functionality that otherwise exists for std::string_view

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what I meant under the couple helper routines umbrella in the PR description 😄

Copy link
Contributor Author

@maxgolov maxgolov Sep 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Practical reason why I'm adding this: I'm working on a swap between nostd:: vs. std:: vs. absl:: ... Abseil will actually be required for Visual Studio 2015 (that's the only way how I can make backport of std::variant work). Some changes like this one, and adding a few headers here and there - are aligning overall code to potentially work with any replacement / substitute for nostd:: . Once this PR is merged, I'll follow-up to refresh my other PR with support for:

  • nostd:: - default for anything vs2017+ and other OS.
  • std:: with msgsl - option for any compiler that supports C++17 and above, for customers who want to statically link the SDK and don't want to dynamically load prebuilt tracers.
  • absl:: - exotic flavor, currently only required for vs2015 (at least for the proper variant backport).

We can also potentially discuss that maybe we should switch from our nostd::variant to absl::variant, as Abseil's variant is at least definitely working on older compilers where ours is failing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I currently have all tests passing for nostd::, std:: + absl:: for Vs2015 in my branch here:
https://github.com/maxgolov/opentelemetry-cpp
I'm doing a matrix build with OpenTelemetry-provided containers vs. Standard Library Containers vs. Abseil (for vs2015 only).

{
std::size_t operator()(const OPENTELEMETRY_NAMESPACE::nostd::string_view &k) const
{
// TODO: for C++17 that has native support for std::basic_string_view it would
// be more performance-efficient to provide a zero-copy hash.
auto s = std::string(k.data(), k.size());
return std::hash<std::string>{}(s);
}
};
} // namespace std
3 changes: 3 additions & 0 deletions api/include/opentelemetry/version.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,4 +12,7 @@

#define OPENTELEMETRY_END_NAMESPACE \
}}

#define OPENTELEMETRY_NAMESPACE opentelemetry :: OPENTELEMETRY_CONCAT(v, OPENTELEMETRY_ABI_VERSION_NO)

// clang-format on
1 change: 1 addition & 0 deletions api/test/metrics/noop_metrics_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#include "opentelemetry/metrics/observer_result.h"
#include "opentelemetry/metrics/sync_instruments.h"

#include <array>
#include <memory>

OPENTELEMETRY_BEGIN_NAMESPACE
Expand Down
7 changes: 5 additions & 2 deletions api/test/nostd/utility_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#include "opentelemetry/nostd/utility.h"

#include <tuple>
#include <type_traits>
#include <vector>

Expand All @@ -20,7 +21,8 @@ TEST(UtilityTest, Data)
std::vector<int> v = {1, 2, 3};
int array[3] = {1, 2, 3};
std::initializer_list<int> list{1, 2, 3};
int x;
int x = 0;
std::ignore = x;

EXPECT_EQ(opentelemetry::nostd::data(v), v.data());
EXPECT_EQ(opentelemetry::nostd::data(array), array);
Expand All @@ -32,7 +34,8 @@ TEST(UtilityTest, Size)
{
std::vector<int> v = {1, 2, 3};
int array[3] = {1, 2, 3};
int x;
int x = 0;
std::ignore = x;

EXPECT_EQ(opentelemetry::nostd::size(v), v.size());
EXPECT_EQ(opentelemetry::nostd::size(array), 3);
Expand Down
4 changes: 2 additions & 2 deletions api/test/trace/key_value_iterable_view_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
#include "opentelemetry/trace/key_value_iterable_view.h"

#include <map>

#include <gtest/gtest.h>
#include <map>
#include "opentelemetry/nostd/type_traits.h"
maxgolov marked this conversation as resolved.
Show resolved Hide resolved

using namespace opentelemetry;

Expand Down
15 changes: 8 additions & 7 deletions exporters/ostream/include/opentelemetry/exporters/ostream/metrics_exporter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#pragma once

#include <iostream>
#include <string>
#include "opentelemetry/sdk/metrics/aggregator/exact_aggregator.h"
#include "opentelemetry/sdk/metrics/aggregator/gauge_aggregator.h"
#include "opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h"
Expand Down Expand Up @@ -82,9 +83,9 @@ class OStreamMetricsExporter final : public sdkmetrics::MetricsExporter
}
else
{
auto vec = agg->get_checkpoint();
int size = vec.size();
int i = 1;
auto vec = agg->get_checkpoint();
size_t size = vec.size();
int i = 1;

sout_ << "\n values : " << '[';

Expand All @@ -104,8 +105,8 @@ class OStreamMetricsExporter final : public sdkmetrics::MetricsExporter
auto boundaries = agg->get_boundaries();
auto counts = agg->get_counts();

int boundaries_size = boundaries.size();
int counts_size = counts.size();
size_t boundaries_size = boundaries.size();
size_t counts_size = counts.size();

sout_ << "\n buckets : " << '[';

Expand Down Expand Up @@ -134,8 +135,8 @@ class OStreamMetricsExporter final : public sdkmetrics::MetricsExporter
auto boundaries = agg->get_boundaries();
auto counts = agg->get_counts();

int boundaries_size = boundaries.size();
int counts_size = counts.size();
size_t boundaries_size = boundaries.size();
size_t counts_size = counts.size();

sout_ << "\n buckets : " << '[';

Expand Down
4 changes: 2 additions & 2 deletions exporters/ostream/include/opentelemetry/exporters/ostream/span_exporter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,8 +146,8 @@ class OStreamSpanExporter final : public sdktrace::SpanExporter

void printAttributes(std::unordered_map<std::string, sdktrace::SpanDataAttributeValue> map)
{
int size = map.size();
int i = 1;
size_t size = map.size();
size_t i = 1;
for (auto kv : map)
{
sout_ << kv.first << ": ";
Expand Down
4 changes: 2 additions & 2 deletions ext/test/zpages/tracez_data_aggregator_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,8 @@ class TracezDataAggregatorTest : public ::testing::Test
void VerifySpanCountsInTracezData(
const std::string &span_name,
const TracezData &aggregated_data,
unsigned int running_span_count,
unsigned int error_span_count,
size_t running_span_count,
size_t error_span_count,
std::array<unsigned int, kLatencyBoundaries.size()> completed_span_count_per_latency_bucket)
{
// Asserts are needed to check the size of the container because they may need
Expand Down
10 changes: 5 additions & 5 deletions ext/test/zpages/tracez_processor_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,8 @@ void UpdateSpans(std::shared_ptr<TracezSpanProcessor> &processor,
*/
bool ContainsNames(const std::vector<std::string> &names,
std::unordered_set<ThreadsafeSpanData *> &running,
unsigned int name_start = 0,
unsigned int name_end = 0,
size_t name_start = 0,
size_t name_end = 0,
bool one_to_one_correspondence = false)
{
if (name_end == 0)
Expand Down Expand Up @@ -97,15 +97,15 @@ bool ContainsNames(const std::vector<std::string> &names,
*/
bool ContainsNames(const std::vector<std::string> &names,
std::vector<std::unique_ptr<ThreadsafeSpanData>> &completed,
unsigned int name_start = 0,
unsigned int name_end = 0,
size_t name_start = 0,
size_t name_end = 0,
bool one_to_one_correspondence = false)
{

if (name_end == 0)
name_end = names.size();

unsigned int num_names = name_end - name_start;
size_t num_names = name_end - name_start;

if (num_names > completed.size() || (one_to_one_correspondence && num_names != completed.size()))
{
Expand Down
2 changes: 1 addition & 1 deletion sdk/include/opentelemetry/sdk/common/atomic_unique_ptr.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ class AtomicUniquePtr
/**
* @return true if the pointer is null
*/
bool IsNull() const noexcept { return ptr_ == nullptr; }
bool IsNull() const noexcept { return ptr_.load() == nullptr; }

/**
* Atomically swap the pointer only if it's null.
Expand Down
3 changes: 3 additions & 0 deletions sdk/include/opentelemetry/sdk/common/empty_attributes.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
#include "opentelemetry/trace/key_value_iterable_view.h"

#include <array>
#include <map>
#include <string>
#include <utility>

OPENTELEMETRY_BEGIN_NAMESPACE
namespace sdk
Expand Down
4 changes: 2 additions & 2 deletions sdk/include/opentelemetry/sdk/metrics/aggregator/exact_aggregator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,8 +145,8 @@ class ExactAggregator : public Aggregator<T>
}
else
{
float position = float(this->checkpoint_.size() - 1) * q;
int ceiling = ceil(position);
float position = float(float(this->checkpoint_.size() - 1) * q);
int ceiling = int(ceil(position));
return this->checkpoint_[ceiling];
}
}
Expand Down
4 changes: 2 additions & 2 deletions sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,8 @@ class HistogramAggregator final : public Aggregator<T>
void update(T val) override
{
this->mu_.lock();
this->updated_ = true;
int bucketID = boundaries_.size();
this->updated_ = true;
size_t bucketID = boundaries_.size();
for (size_t i = 0; i < boundaries_.size(); i++)
{
if (val < boundaries_[i]) // concurrent read is thread-safe
Expand Down
2 changes: 1 addition & 1 deletion sdk/include/opentelemetry/sdk/metrics/aggregator/sketch_aggregator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ class SketchAggregator final : public Aggregator<T>
idx = iter->first;
count += iter->second;
}
return round(2 * pow(gamma, idx) / (gamma + 1));
return (T)(round(2 * pow(gamma, idx) / (gamma + 1)));
maxgolov marked this conversation as resolved.
Show resolved Hide resolved
}

/**
Expand Down
8 changes: 4 additions & 4 deletions sdk/test/common/circular_buffer_benchmark.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,8 +102,8 @@ static void RunSimulation(Buffer &buffer, int num_threads, int n) noexcept
static void BM_BaselineBuffer(benchmark::State &state)
{
const size_t max_elements = 500;
auto num_threads = state.range(0);
const int n = N / num_threads;
const int num_threads = static_cast<int>(state.range(0));
const int n = static_cast<int>(N / num_threads);
BaselineCircularBuffer<uint64_t> buffer{max_elements};
for (auto _ : state)
{
Expand All @@ -116,8 +116,8 @@ BENCHMARK(BM_BaselineBuffer)->Arg(1)->Arg(2)->Arg(4);
static void BM_LockFreeBuffer(benchmark::State &state)
{
const size_t max_elements = 500;
auto num_threads = state.range(0);
const int n = N / num_threads;
const int num_threads = static_cast<int>(state.range(0));
const int n = static_cast<int>(N / num_threads);
CircularBuffer<uint64_t> buffer{max_elements};
for (auto _ : state)
{
Expand Down
1 change: 1 addition & 0 deletions sdk/test/common/circular_buffer_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#include "opentelemetry/sdk/common/circular_buffer.h"

#include <algorithm>
#include <cassert>
#include <random>
#include <thread>
Expand Down