-
Notifications
You must be signed in to change notification settings - Fork 438
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
potential fix for std::variant implicit conversion in C++17 STL #734
Conversation
Codecov Report
@@ Coverage Diff @@
## main #734 +/- ##
=======================================
Coverage 96.00% 96.01%
=======================================
Files 176 176
Lines 7183 7199 +16
=======================================
+ Hits 6896 6912 +16
Misses 287 287
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm Okay with this as a temporary measure. I think we could have handled it in the AttributeValue
class. I think the issue is that since it's a non-owning and always creates a string_view
- this causes some odd issues with temporary arrays of chars..
This is not related to |
@@ -150,7 +150,7 @@ TEST(OStreamLogExporter, LogWithStringAttributesToCerr) | |||
auto record = exporter->MakeRecordable(); | |||
|
|||
// Set resources for this log record only of type <string, string> | |||
record->SetResource("key1", "val1"); | |||
record->SetResource("key1", std::string("val1")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we provide an explicit overload to SetResource
with const char *
to avoid potential future error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like that. I'm proposing to re-add const char *
to variant. Just verified that it works, and including it in the Abseil Variant PR.
This re-add of const char *
will give us much nicer user / developer experience, when we get to C++17 standard lib impl of variant.
Just to understand, we are seeing this issue both with C++17 STL and absl::variant, and mpark::variant handles this nicely ? |
Sure closing this PR. |
Fixes ##733
Changes
@maxgolov This is not for merge, but to showcase the potential changes needed to fix #733, As mentioned in the comment there, this is not related to resource sdk.
CHANGELOG.md
updated for non-trivial changes