-
Notifications
You must be signed in to change notification settings - Fork 126
Conversation
Signed-off-by: Isaac Hier <[email protected]>
Signed-off-by: Isaac Hier <[email protected]>
Signed-off-by: Isaac Hier <[email protected]>
src/jaegertracing/DynamicLoad.cpp
Outdated
*errorCategory = static_cast<const void*>( | ||
if (std::strcmp(opentracing_version, OPENTRACING_VERSION) != 0 || | ||
std::strcmp(opentracing_abi_version, OPENTRACING_ABI_VERSION) != 0) { | ||
*error_category = static_cast<const void*>( |
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 think you want std::strcmp(opentracing_version, OPENTRACING_VERSION) != 0
in the comparison.
The point of adding the ABI version was to allow the plugin to work with other versions of OT if there's no ABI breakage.
The version is only passed in for the case that you might have some sort of incompatible semantic change in the future.
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.
Good point.
src/jaegertracing/DynamicLoad.h
Outdated
const char* opentracing_abi_version, | ||
const void** error_category, | ||
void* error_message, | ||
void** tracer_factory); |
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.
You don't need this function exposed. The macro OPENTRACING_DECLARE_IMPL_FACTORY declares a variable function pointer (See for example how it's done in the LS tracer: https://github.com/lightstep/lightstep-tracer-cpp/blob/master/src/dynamic_load.cpp#L6)
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.
Thanks for the example!
Signed-off-by: Isaac Hier <[email protected]>
Signed-off-by: Isaac Hier <[email protected]>
…p into update-hunter
@rnburn do you mind checking if everything looks OK? Thanks in advance. |
src/jaegertracing/DynamicLoad.cpp
Outdated
{ | ||
assert(errorCategory != nullptr); | ||
assert(tracerFactory != nullptr); | ||
#ifndef JAEGERTRACING_WITH_YAML_CPP | ||
*errorCategory = | ||
static_cast<const void*>(&opentracing::dynamic_load_error_category()); | ||
static_cast<const void*>(&opentracing::dynamic_load_errorCategory()); |
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.
dynamic_load_errorCategory?
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.
Lol I was camelcasing this stuff an must have messed that up. Surprised this built...
Signed-off-by: Isaac Hier <[email protected]>
Which problem is this PR solving?
Short description of the changes