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

WITH_STL shouldn't try importing gsl::span if std::span is not available. #813

Closed
lalitb opened this issue Jun 1, 2021 · 10 comments · Fixed by #1167
Closed

WITH_STL shouldn't try importing gsl::span if std::span is not available. #813

lalitb opened this issue Jun 1, 2021 · 10 comments · Fixed by #1167
Assignees
Labels
bug Something isn't working build and test help wanted Good for taking. Extra help will be provided by maintainers

Comments

@lalitb
Copy link
Member

lalitb commented Jun 1, 2021

As discussed in #742:
If the WITH_STL cmake option is enabled, the build should be using std::span if available (for C++20), and fall back to nostd::span instead of gsl::span ( or perhaps fail the build). Use gsl::span only if developers explicitly specify the WITH_GSL build option.

@lalitb lalitb added bug Something isn't working build and test labels Jun 1, 2021
@maxgolov
Copy link
Contributor

maxgolov commented Jun 2, 2021

I already added a fix for that here:

Could you please take a look at my PR? #771

@maxgolov
Copy link
Contributor

maxgolov commented Jun 2, 2021

I added my comments to #742 here - #742 (comment)

@maxgolov
Copy link
Contributor

maxgolov commented Jun 3, 2021

I'll assign to myself. I believe I have a reasonable fix in my bigger variant PR. Feedback is welcomed, in case if we want to decouple STDLIB vs GSL. But it's been described in the document here that we fallback to gsl::span for C++17 , because std::span is only available starting from C++20.

@maxgolov
Copy link
Contributor

maxgolov commented Jun 4, 2021

I would like to test this on latest build. Although we enable the option by-default for STDLIB build - we turn it off in case if customer's build system does not have gsl header. So the issue is largely alleviated: that way we do not force the customers to use the header.

Perhaps we should've switched our nostd::span implementation (aliasing it) to gsl::span. Min bar for gsl::span, however, is C++14. So switching to GSL may not work too well for gcc-4.8.x . I have not personally tested if it'd work specifically with that super-old compiler. Need more time to research.

Main motivation of defaulting to GSL span rather than implementing our own nostd::span :

  • GSL span is better tested (more flight-hours on that).
  • GSL span is endorsed by CppCoreGuidelines.
  • GSL span is approved in various secure lifecycle development processes.
  • less maintenance headache (we do not want to maintain our own mini-STL library in OpenTelemetry).

@owent
Copy link
Member

owent commented Oct 15, 2021

The gsl::span implementation in gsl-lite support gcc-4.8.x. Should we use it as gsl for old compilers?

@lalitb
Copy link
Member Author

lalitb commented Oct 15, 2021

The gsl::span implementation in gsl-lite support gcc-4.8.x. Should we use it as gsl for old compilers?

What's the current behavior? I thought if gsl-lite is installed, and HAVE_GSL is enabled, api/sdk will use nostd::variant from gsl-lite. If this is not so, we can add support for that.

@github-actions
Copy link

This issue was marked as stale due to lack of activity. It will be closed in 7 days if no furthur activity occurs.

@github-actions github-actions bot added the Stale label Dec 15, 2021
@lalitb lalitb removed the Stale label Dec 15, 2021
@maxgolov
Copy link
Contributor

@owent @lalitb - could you please folks take over the ownership of this issue? My main focus is on another project domain at the moment, and my ability to contribute to OpenTelemetry is limited.

@lalitb lalitb added the help wanted Good for taking. Extra help will be provided by maintainers label Dec 29, 2021
@lalitb
Copy link
Member Author

lalitb commented Dec 29, 2021

Thanks, @maxgolov, have unassigned you and added a help-wanted tag. I will take it over if no one picks it. The immediate need for this issue would be not using gsl::span with WITH_STL flag, and then we can have a separate issue to introduce the WITH_GSL cmake flag.

@esigo
Copy link
Member

esigo commented Dec 29, 2021

@lalitb I can take it over.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working build and test help wanted Good for taking. Extra help will be provided by maintainers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants