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.

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

Add back port of std::span #21

Merged
merged 24 commits into from
Jan 7, 2020
Merged

Add back port of std::span #21

merged 24 commits into from
Jan 7, 2020

Conversation

rnburn
Copy link
Contributor

@rnburn rnburn commented Jan 1, 2020

Like std::string_view this is another basic non-owning vocabulary type.

Copy link

@jmacd jmacd left a comment

Choose a reason for hiding this comment

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

(It's mildly confusing that span here does not refer to an OTel Span!)

@rnburn
Copy link
Contributor Author

rnburn commented Jan 2, 2020

But at least it's in a separate namespace -- and it's worth it to match the names in the standard library

T *end() const noexcept { return data_ + extent_; }

private:
// Note: matches libstdc++'s layout for std::span
Copy link
Member

Choose a reason for hiding this comment

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

Curious - is this a best effort or we want to keep the layout compatibility as a promise?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just a best effort. It also varies by standard library. For example, libc++ uses a different order
https://github.com/llvm-mirror/libcxx/blob/master/include/span#L520-L521

{
if (count != Extent)
{
std::terminate();
Copy link
Member

Choose a reason for hiding this comment

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

Would this introduce a dependency on stdlib?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By stdlib, do you mean the standard c library?

std::terminate would be defined in the standard c++ library; and unless, you change the terminate handler with std::set_terminate, it will forward to abort

Copy link
Member

Choose a reason for hiding this comment

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

libstdc++

I remember we discussed about whether we should have dependency on the standard c++ library but we haven't got the conclusion. I'm curious to know if someone could see the downside of having libstdc++ dependency (e.g. static link scenario?)

https://github.com/grpc/proposal/blob/master/L59-core-allow-cppstdlib.md

@rnburn rnburn merged commit c85b20b into open-telemetry:master Jan 7, 2020
0x4b pushed a commit to 0x4b/opentelemetry-cpp that referenced this pull request Aug 19, 2020
commenting out trace state in span context
GerHobbelt pushed a commit to GerHobbelt/opentelemetry-cpp that referenced this pull request Jun 21, 2024
[MISC] Use set -e on all shell scripts and pass shellcheck --severity…
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants