Skip to content
This repository has been archived by the owner on Aug 30, 2022. It is now read-only.

Implement SpanContext::Clone #138

Merged
merged 1 commit into from
Nov 21, 2018
Merged

Implement SpanContext::Clone #138

merged 1 commit into from
Nov 21, 2018

Conversation

meastman
Copy link
Contributor

This was method added to the opentracing API as a pure virtual method in opentracing/opentracing-cpp#56. Without implementing this method, the jaeger client library cannot be used with recent (post-August-2018) versions of opentracing.

@isaachier
Copy link
Contributor

@meastman looks good. Travis CI has this error:

In file included from /home/travis/build/jaegertracing/jaeger-client-cpp/src/jaegertracing/SpanContext.cpp:17:0:
/home/travis/build/jaegertracing/jaeger-client-cpp/src/jaegertracing/SpanContext.h:167:10: error: ‘unique_ptr’ in namespace ‘std’ does not name a template type
     std::unique_ptr<opentracing::SpanContext> Clone() const noexcept

Try including <memory> in SpanContext.h.

@@ -164,6 +164,12 @@ class SpanContext : public opentracing::SpanContext {
forEachBaggageItem(f);
}

std::unique_ptr<opentracing::SpanContext> Clone() const noexcept
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a lock guard here please:

std::lock_guard<std::mutex> lock(_mutex);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

This was added to the opentracing API in
opentracing/opentracing-cpp#56

Signed-off-by: Matt Eastman <[email protected]>
@meastman
Copy link
Contributor Author

Try including <memory> in SpanContext.h.

Added.

Copy link
Contributor

@isaachier isaachier left a comment

Choose a reason for hiding this comment

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

LGTM thanks for the PR.

@isaachier isaachier merged commit cefa941 into jaegertracing:master Nov 21, 2018
@meastman
Copy link
Contributor Author

LGTM thanks for the PR.

Thanks for the quick review!

@meastman meastman deleted the add-spancontext-clone branch November 21, 2018 22:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants