Visual Studio compiler warnings clean-up

[maxgolov]

Partially addresses #249

Main goals:

• resolve compiler warnings that show up in Visual Studio 2015/2017/2019.
• make sure we use size_t instead of int for any index variable, even in tests. It's a bad coding practice to use signed integers for array index.
• add a few headers to facilitate the merger of std:: and absl:: alternatives to nostd:: classes where needed.
• couple helper routines.

I'll send in a separate PR for setting up CI for Visual Studio 2015, with Warnings Level 4 enabled, and subsequently breaking the build if somebody introduces a warning. That separate PR will also depend on replacing nostd::variant by absl::variant, because our built-in backport of nostd::variant is not good ( see #314 ). Currently I'm testing out that flow in my fork.

What is not addressed yet:

• Metrics API introduced a bunch of warnings, like X inherits Y via dominance ( ref. https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-2-c4250?view=vs-2019 ) :

4>  C:\work\opentelemetry-cpp\sdk\include\opentelemetry/sdk/metrics/instrument.h(50): note: see declaration of 'opentelemetry::v0::sdk::metrics::Instrument::GetKind'
4>C:\work\opentelemetry-cpp\sdk\include\opentelemetry/sdk/metrics/sync_instruments.h(64): warning C4250: 'opentelemetry::v0::sdk::metrics::BoundCounter<T>': inherits 'opentelemetry::v0::sdk::metrics::BoundSynchronousInstrument<T>::opentelemetry::v0::sdk::metrics::BoundSynchronousInstrument<T>::unbind' via dominance

Since this will require refactor in the Metrics API, I am not touching those warnings, although they are actually "Level 2" warnings.

[codecov]

Codecov Report

Merging #315 into master will increase coverage by 0.00%.
The diff coverage is 95.45%.

@@           Coverage Diff           @@
##           master     #315   +/-   ##
=======================================
  Coverage   94.59%   94.60%           
=======================================
  Files         146      146           
  Lines        6629     6633    +4     
=======================================
+ Hits         6271     6275    +4     
  Misses        358      358           

Impacted Files	Coverage Δ
api/include/opentelemetry/nostd/string_view.h	97.36% <ø> (ø)
api/test/metrics/noop_metrics_test.cc	88.63% <ø> (ø)
api/test/trace/key_value_iterable_view_test.cc	100.00% <ø> (ø)
ext/test/zpages/tracez_data_aggregator_test.cc	97.34% <ø> (ø)
...nclude/opentelemetry/sdk/common/empty_attributes.h	100.00% <ø> (ø)
...lemetry/sdk/metrics/aggregator/sketch_aggregator.h	69.76% <0.00%> (ø)
sdk/test/common/circular_buffer_test.cc	100.00% <ø> (ø)
api/include/opentelemetry/context/context.h	98.18% <100.00%> (ø)
api/test/nostd/utility_test.cc	100.00% <100.00%> (ø)
...opentelemetry/exporters/ostream/metrics_exporter.h	98.66% <100.00%> (ø)
... and 5 more

[reyang]

LGTM.

[maxgolov]

Formatter on my Ubuntu 20.xx clang-format version 10.0.0-4ubuntu1 produces different formatting result with the same .clang-format file than the formatter that we run in our CI clang-format version 8.0.1-9 (tags/RELEASE_801/final) 😁

[pyohannes]

One question, otherwise this looks ok to me.

[pyohannes]

Is this necessary for VS2015?

[maxgolov]

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

[maxgolov]

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

[maxgolov]

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.

[maxgolov]

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).

[g-easy]

I would prefer to keep the loop variables as (signed) ints and remove the warning flag instead.

Context: https://google.github.io/styleguide/cppguide.html#Integer_Types

[maxgolov]

I would prefer to keep the loop variables as (signed) ints and remove the warning flag instead.

Context: https://google.github.io/styleguide/cppguide.html#Integer_Types

This same rule says When appropriate, you are welcome to use standard types like size_t and ptrdiff_t.. It is not appropriate to use signed integer as array index, for example.

[ThomsonTan]

LGTM.