-
Notifications
You must be signed in to change notification settings - Fork 114
Conversation
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.
Overall really good.
# Increment ABI version for any ABI-breaking change. | ||
# | ||
# Also, whenever the ABI is between versions and in development | ||
# suffix the ABI version number with "_unstable". |
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.
Might be worth adding this comment to some sort of release documentation. Jaeger repos usually have a RELEASE.md
for this type of thing.
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.
Sounds good. I'll add a RELEASE.md
.
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.
Added.
test/multiple_tracer_link_test.cpp
Outdated
@@ -0,0 +1,3 @@ | |||
// Contains nothing, but links in tracer_a.o and tracer_b.o to verify that | |||
// there's no multiple definition error from OpenTracingMakeTracerFactory. |
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 wonder if this is guaranteed to cause a compilation error. Might be worth using the function pointer for some trivial operation to ensure the symbol isn't discarded during linking.
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.
Done.
|
||
return 0; | ||
} catch (const std::bad_alloc&) { |
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.
Out of curiosity, why'd you change to try
/catch
? If you expect other types of exception, make sure to catch std::exception
or catch (...)
.
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 added the try/catch because it assigns to the string error_message
, which can allocate memory. std::bad_alloc
should be the only exception that could be thrown.
Sets up conventions for versioning the ABI to allow for less strict checking when dynamically loading tracers.
OpenTracingMakeTracerFactory
function prototype to take an ABI version in addition to an API version.OpenTracingMakeTracerFactory
should fail only if the ABI version is different. (The API version is still passed in case there's ever some sort of change that breaks semantic compatibility).I also plan to put in an additional PR with some CI coverage that will help to detect ABI breakage.
Fixes #81