Skip to content

Patches to enable Visual Studio 2015 - Update 2 support #565

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

Merged
lalitb merged 4 commits into from
Feb 10, 2021
Merged

Patches to enable Visual Studio 2015 - Update 2 support #565

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f48431c
Patches to enable Visual Studio 2015 - Update 2 support
maxgolov Feb 5, 2021
c95dbd8
Submit formatting changes + disable format for Abseil variant impleme…
maxgolov Feb 5, 2021
2a4eaf1
Add README.md
maxgolov Feb 5, 2021
00bd76c
Convert to YAML
maxgolov Feb 5, 2021
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
2 changes: 1 addition & 1 deletion api/include/opentelemetry/context/context.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ class Context

// Builds a data list with just a key and value, so it will just be the head
// and returns that head.
DataList(nostd::string_view key, ContextValue value)
DataList(nostd::string_view key, const ContextValue &value)
{
key_ = new char[key.size()];
key_length_ = key.size();
Expand Down
23 changes: 15 additions & 8 deletions api/include/opentelemetry/context/runtime_context.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ class Token

// A constructor that sets the token's Context object to the
// one that was passed in.
Token(Context context) : context_(context) {}
Token(const Context &context) : context_(context) {}

const Context context_;
};
Expand All @@ -49,7 +49,7 @@ class RuntimeContextStorage
* @param the new current context
* @return a token for the new current context. This never returns a nullptr.
*/
virtual nostd::unique_ptr<Token> Attach(Context context) noexcept = 0;
virtual nostd::unique_ptr<Token> Attach(const Context &context) noexcept = 0;

/**
* Detach the context related to the given token.
Expand All @@ -59,7 +59,7 @@ class RuntimeContextStorage
virtual bool Detach(Token &token) noexcept = 0;

protected:
nostd::unique_ptr<Token> CreateToken(Context context) noexcept
nostd::unique_ptr<Token> CreateToken(const Context &context) noexcept
{
return nostd::unique_ptr<Token>(new Token(context));
}
Expand All @@ -82,7 +82,7 @@ class RuntimeContext

// Sets the current 'Context' object. Returns a token
// that can be used to reset to the previous Context.
static nostd::unique_ptr<Token> Attach(Context context) noexcept
static nostd::unique_ptr<Token> Attach(const Context &context) noexcept
{
return GetRuntimeContextStorage()->Attach(context);
}
Expand All @@ -98,7 +98,7 @@ class RuntimeContext
// mind that the current RuntimeContext will not be changed, and the new
// context will be returned.
static Context SetValue(nostd::string_view key,
ContextValue value,
const ContextValue &value,
Context *context = nullptr) noexcept
{
Context temp_context;
Expand Down Expand Up @@ -204,7 +204,7 @@ class ThreadLocalContextStorage : public RuntimeContextStorage

// Sets the current 'Context' object. Returns a token
// that can be used to reset to the previous Context.
nostd::unique_ptr<Token> Attach(Context context) noexcept override
nostd::unique_ptr<Token> Attach(const Context &context) noexcept override
{
GetStack().Push(context);
return CreateToken(context);
Expand Down Expand Up @@ -254,7 +254,7 @@ class ThreadLocalContextStorage : public RuntimeContextStorage

// Pushes the passed in context pointer to the top of the stack
// and resizes if necessary.
void Push(Context context) noexcept
void Push(const Context &context) noexcept
{
size_++;
if (size_ > capacity_)
Expand All @@ -275,7 +275,14 @@ class ThreadLocalContextStorage : public RuntimeContextStorage
Context *temp = new Context[new_capacity];
if (base_ != nullptr)
{
std::copy(base_, base_ + old_size, temp);
// vs2015 does not like this construct considering it unsafe:
Copy link
Copy Markdown
Contributor

@ThomsonTan ThomsonTan Feb 9, 2021

Choose a reason for hiding this comment

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

Check Visual Studio 2015 compiler like via _MSC_VER to separate the workaround for it?

Copy link
Copy Markdown
Contributor Author

@maxgolov maxgolov Feb 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Does std::copy itself perform a bounds checking? i.e. what if new temp size - new_capacity is less than the old capacity? Since it's a raw pointer, I think the old code had a problem: it was copying beyond the allocated buffer, buffer overflow.

// - std::copy(base_, base_ + old_size, temp);
// Ref.
// https://stackoverflow.com/questions/12270224/xutility2227-warning-c4996-std-copy-impl
for (size_t i = 0; i < (std::min)(old_size, new_capacity); i++)
{
temp[i] = base_[i];
}
delete[] base_;
}
base_ = temp;
Expand Down
3 changes: 3 additions & 0 deletions api/include/opentelemetry/nostd/absl/.clang-format
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# Disable formatting for Google Abseil library snapshot
DisableFormat: true
SortIncludes: false
9 changes: 9 additions & 0 deletions api/include/opentelemetry/nostd/absl/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# Notes on Abseil Variant implementation

This is a snapshot of Abseil Variant `absl::variant` from Abseil `v2020-03-03#8`.
Comment thread
maxgolov marked this conversation as resolved.

This code is required to compile OpenTelemetry code in vs2015 because MPark Variant implementation is not compatible with vs2015.

Build option `HAVE_ABSEIL_VARIANT` allows to enable the build with `absl::variant`, `absl::get` and `absl::visit` as defalt implementation for `nostd::` classes.
Comment on lines +5 to +7
Copy link
Copy Markdown
Contributor

@pyohannes pyohannes Feb 9, 2021

Choose a reason for hiding this comment

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

Is there a reason we have to ship two variant implementations? If absl is needed for VS2015 (and we decide we want to support that), can't we just generally use the absl implementation (instead of the MPark one)?

Copy link
Copy Markdown
Contributor Author

@maxgolov maxgolov Feb 9, 2021

Choose a reason for hiding this comment

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

I'd like to take a two-staged approach. We should use Abseil everywhere, but I'd like to keep it under the build option right now until I fully test all existing exporters with it. Within the same OS class the ABI compatibility is retained irrespective of what implementation we take, e.g. Mpark works for Linux and Mac no problem. Whereas Abseil works for Windows. I want to send a follow-up PR that adds a build loop for Windows to switch over to Abseil for Variant. Then we can do this for Linux and Mac.

Copy link
Copy Markdown
Contributor Author

@maxgolov maxgolov Feb 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think long-term before v1.0 GA we should switch to Abseil for variant. That'd be great for another reason: we can get rid of Boost license in our list of licenses because only MPark variant is Boost-licensed, whereas Abseil is also Apache License v2.0 as the rest of code. But I'd like to do this switch in a separate PR.

Copy link
Copy Markdown
Contributor Author

@maxgolov maxgolov Feb 9, 2021

Choose a reason for hiding this comment

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

#68

Copy link
Copy Markdown
Contributor Author

@maxgolov maxgolov Feb 9, 2021

Choose a reason for hiding this comment

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

#68

Copy link
Copy Markdown
Contributor

@pyohannes pyohannes Feb 10, 2021

Choose a reason for hiding this comment

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

Makes sense. Can you add an additional issue for this (apart from the license one), otherwise this might be lost when just looking at issue titles.

I'd be fine with switching to absl as default, having MPark under a switch (then our CI pipeline would verify how it's working). However, I assume you expect changes to be necessary for it to work on Linux.

Copy link
Copy Markdown
Member

@lalitb lalitb Feb 10, 2021

Choose a reason for hiding this comment

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

Have created an issue for this #572 , and assigned to @maxgolov . Feel free to update with relevant details on it.


Going forward it makes sense to use `absl::variant` as default implementation for Windows OS and vs2015, vs2017, vs2019 and newer compilers.
Loading