Using STD library for API surface #374
Conversation
Codecov Report
@@ Coverage Diff @@
## master #374 +/- ##
==========================================
+ Coverage 94.40% 94.44% +0.04%
==========================================
Files 187 187
Lines 8234 8233 -1
==========================================
+ Hits 7773 7776 +3
+ Misses 461 457 -4
|
jsuereth
left a comment
jsuereth
left a comment
There was a problem hiding this comment.
This looks pretty good. I'm taking a crack at seeing if I can expose the same flexibility in the bazel-build without too much effort.
Keep third_party/json reserved for CMake submodule build. Remove top-level cmake/TBD as it is not needed and not used.
|
@jsuereth @lalitb @ThomsonTan - I think I'm kinda done with this, with the following follow-ups that I will send in a separate PR:
The only part that I feel a bit uneasy about is I also owe a spreadsheet with benchmark results. I added the benchmark, btw. But it'd be nice to illustrate the real benefits in a real-world test case. There could be some further optimization applied on API surface, when we know that a collection is one of standard types and when exporter can operate on a standard type, not needing an intermediate |
jsuereth
left a comment
jsuereth
left a comment
There was a problem hiding this comment.
Would like to cleanup all the benchmark code, but otherwise looking forward to pushing this to the next level!
lalitb
left a comment
lalitb
left a comment
There was a problem hiding this comment.
LGTM overall. Somewhat large PR, but thanks for documenting the changes, and clarifying ABI related issues we will have with stl.
| #include <type_traits> | ||
| #ifdef HAVE_CPP_STDLIB | ||
| # include "opentelemetry/std/span.h" | ||
| #else |
There was a problem hiding this comment.
Should there be check forHAVE_ABSEIL macro too, and include absl/span.h if set.
There was a problem hiding this comment.
I was unconditionally using either std::span or gsl::span since GSL span implementation is guaranteed to be 100% conformant, whereas absl::Span is not guarnateed to be conformant. I added a brief description to the markdown document.... Basically for now I'd use variant from absl::variant , because there is no other option that would compile with Visual Studio 2015. But not using absl for Span. I can see if we can use that in a separate PR.
There was a problem hiding this comment.
| #include <string> | ||
| #ifdef HAVE_CPP_STDLIB | ||
| # include "opentelemetry/std/string_view.h" | ||
| #else |
There was a problem hiding this comment.
Not yet. I'm not adding full HAVE_ABSEIL support, planning to only use that for absl::variant with Visual Studio 2015. For string_view we only support nostd::string_view and std::string_view. No support for absl::string_view here.
In all other build systems we prefer the following - in the following order, when HAVE_CPP_STDLIB feature is enabled:
- prefer standard
std::classes in C++14, C++17, C++20 - prefer
gsl::spanfor C++11, C++14, C++17. GSL is the only fully conforming implementation.absl::Spanis NOT 100% standards -conforming. I described this in the markdown doc. - prefer
absl::variantfor v2015: technically this is needed only for C++11 on Windows with vs2015. C++17 already hasstd::variant. We should eventually remove MPark variant (nostd::variant). And borrowabsl::variantinstead of MPark variant.
Otherwise our builds will not work with anything that is compiled with vs2015. I kept MPark variant for now. Basically, Abseil is only needed for variant, and absl::variant itself is only needed for vs2015 support. I did foundation work to resolve these issues: #215 #314
I'll add vs2015 in CI in a separate PR. There is no default runner with vs2015 on GitHub. Installing build tools of that may take considerable amount of time in CI. Gotta be creative to sort out how to deploy it the fastest way.
maxgolov
added
the
pr:please-merge
> Merge [open-telemetry#374](open-telemetry#374) and rebase from master again.
> Merge [open-telemetry#374](open-telemetry#374) and rebase from master again.
Update dependency rules_cue to v0.13.2
I created a separate design doc to explain the logic and what's done in
docs/building-with-stdlib.md@jsuereth