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

Jump to bottom

importing gsl::span if std::span is not available #1167

Merged
merged 7 commits into from
Jan 25, 2022
Merged

importing gsl::span if std::span is not available #1167

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c676c16
importing gsl::span if std::span is not available
esigo Jan 1, 2022
3da1084
Merge branch 'main' into importing-gsl--span-if-std--span-is-not-avai…
esigo Jan 9, 2022
9af60ec
Merge branch 'main' into importing-gsl--span-if-std--span-is-not-avai…
esigo Jan 12, 2022
3071ae7
stop defining have_gsl
esigo Jan 16, 2022
32cd796
Merge branch 'main' into importing-gsl--span-if-std--span-is-not-avai…
esigo Jan 16, 2022
f6a32fc
Merge branch 'main' into importing-gsl--span-if-std--span-is-not-avai…
esigo Jan 20, 2022
7350a45
Merge branch 'main' into importing-gsl--span-if-std--span-is-not-avai…
lalitb Jan 22, 2022
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
30 changes: 17 additions & 13 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,9 @@ else()
)
endif()

option(WITH_STL "Whether to use Standard Library for C++latest features" OFF)
option(WITH_STL "Whether to use Standard Library for C++ latest features" OFF)
option(WITH_GSL
"Whether to use Guidelines Support Library for C++ latest features" OFF)

option(WITH_ABSEIL "Whether to use Abseil for C++latest features" OFF)

Expand Down Expand Up @@ -137,18 +139,6 @@ if(WITH_STL)
# (absl::variant or std::variant) in variant unit test code is consistent with
# the global project build definitions.
add_definitions(-DHAVE_CPP_STDLIB)
add_definitions(-DHAVE_GSL)

# Guidelines Support Library path. Used if we are not on not get C++20.
#
# TODO: respect WITH_ABSEIL as alternate implementation of std::span
find_package(Microsoft.GSL QUIET)
if(TARGET Microsoft.GSL::GSL)
list(APPEND CORE_RUNTIME_LIBS Microsoft.GSL::GSL)
else()
set(GSL_DIR third_party/ms-gsl)
include_directories(${GSL_DIR}/include)
endif()

# Optimize for speed to reduce the hops
if(CMAKE_CXX_COMPILER_ID STREQUAL "MSVC")
Expand All @@ -165,6 +155,20 @@ if(WITH_STL)
endif()
endif()

if(WITH_GSL)
add_definitions(-DHAVE_GSL)
Copy link
Member

@lalitb lalitb Jan 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Just to clarify- with this PR, GSL library will only be used if the WITH_GSL flag is enabled?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes it is enabled only when the user explicitly asks for it.

Copy link
Member

@lalitb lalitb Jan 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, I was thinking about this part in cmake:

opentelemetry-cpp/api/CMakeLists.txt

Lines 48 to 51 in 05d43d6

if(WITH_STL)
message("Building with standard library types...")
target_compile_definitions(opentelemetry_api INTERFACE HAVE_CPP_STDLIB
HAVE_GSL)

As it looks we are enabling HAVE_GSL macro with WITH_STL option, which would mean try using gsl::span from submodule if STL doesn't have std::span implementation.

Copy link
Member Author

@esigo esigo Jan 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

that is covered in

opentelemetry-cpp/api/include/opentelemetry/std/span.h

Lines 29 to 31 in bc1b469

# if !__has_include(<gsl/gsl>)
# undef HAVE_GSL
# endif
as the header is not available when the WITH_GSL is off.
We shouldn't define it though, I cleaned it from the CMake file.

Copy link
Member

Choose a reason for hiding this comment

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

ok, thanks for the clarification.


# Guidelines Support Library path. Used if we are not on not get C++20.
#
find_package(Microsoft.GSL QUIET)
if(TARGET Microsoft.GSL::GSL)
list(APPEND CORE_RUNTIME_LIBS Microsoft.GSL::GSL)
else()
set(GSL_DIR third_party/ms-gsl)
include_directories(${GSL_DIR}/include)
endif()
endif()

option(WITH_OTLP "Whether to include the OpenTelemetry Protocol in the SDK" OFF)
option(WITH_ZIPKIN "Whether to include the Zipkin exporter in the SDK" OFF)

Expand Down
3 changes: 1 addition & 2 deletions api/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,7 @@ endif()

if(WITH_STL)
message("Building with standard library types...")
target_compile_definitions(opentelemetry_api INTERFACE HAVE_CPP_STDLIB
HAVE_GSL)
target_compile_definitions(opentelemetry_api INTERFACE HAVE_CPP_STDLIB)
else()
message("Building with nostd types...")
endif()
Expand Down
4 changes: 2 additions & 2 deletions api/include/opentelemetry/std/span.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
#if defined __has_include
# if __has_include(<version>) // Check for __cpp_{feature}
# include <version>
# if defined(__cpp_lib_span)
# if defined(__cpp_lib_span) && __cplusplus > 201703L
Copy link
Member

Choose a reason for hiding this comment

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

Why is this check required?

Copy link
Member Author

Choose a reason for hiding this comment

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

because __cpp_lib_span is defined for C++17 on g++-10:g++-10 (Ubuntu 10.3.0-1ubuntu1~20.04) 10.3.0.
I use these CMake options for build:
-DCMAKE_CXX_COMPILER=g++-10 -DCMAKE_CXX_STANDARD=17

Copy link
Member

Choose a reason for hiding this comment

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

Which should be ok right? If the HAVE_CPP_STDLIB option is specified, and the standard library provides the implementation ( stable or experimental) it should get selected. What is the behavior now with these changes on g++-10/C++17 - compilation failure as std::span is not available?

Copy link
Member Author

Choose a reason for hiding this comment

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

The behavior for g++-10/C++17 is that nostd::span will be picked if the WITH_GSL option was not used. I think we shouldn't use std::span if the user explicitly asks for C++17.
Shall I enable std::span if C++17 was used and std::span is available?

# define HAVE_SPAN
# endif
# endif
Expand All @@ -21,7 +21,7 @@
# define HAVE_SPAN
# endif
# // Check for other compiler span implementation
# if !defined(_MSVC_LANG) && __has_include(<span>)
# if !defined(_MSVC_LANG) && __has_include(<span>) && __cplusplus > 201703L
// This works as long as compiler standard is set to C++20
# define HAVE_SPAN
# endif
Expand Down
1 change: 1 addition & 0 deletions sdk/include/opentelemetry/sdk/instrumentationlibrary/instrumentation_library.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

#pragma once

#include <string>
#include "opentelemetry/nostd/string_view.h"
#include "opentelemetry/nostd/unique_ptr.h"
#include "opentelemetry/version.h"
Expand Down