-
Notifications
You must be signed in to change notification settings - Fork 290
Hide all symbols except explicitly exported with CASS_EXPORT. #355
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
Conversation
Hi @API92, thanks for your contribution! In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. Sincerely, |
It fixes crash (or abort failure in Debug configuration) when some .so uses other version (1.1) of rapidjson from /usr/include and override some (not all) rapidjson methods compiled in libcassandra.so. This reproduced when connecting to scylladb. In Debug configuration it looks like rapidjson::Document::StartObject (from /usr/include/rapidjson/...) called with wrong this pointer from Parser (from third_party/rapidjson/...), but preprocessor output shows only sources from third_party/rapidjson.
Thank you @API92 for signing the Contribution License Agreement. Cheers, |
Thanks for the patch. I'll take a look at this sometime this week. |
# define CASS_IMPL_EXPORT __declspec(dllexport) | ||
# else | ||
# define CASS_IMPL_EXPORT __declspec(dllexport) | ||
# endif |
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.
The if and else bodies are identical. Why have this if for CASS_BUILDING?
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.
This code copied from include/cassandra.h. In commit fb4c1a6 the message says that it solves some Windows warnings. But I can rewrite this code with single #define.
Really, CASS_IMPL_EXPORT needed only for exporting some symbols for unit tests. Maybe would it be better to add macro CASS_BUILDING_TESTS and only if it is set, then to define CASS_IMPL_EXPORT?
# if defined(CASS_BUILDING) | ||
# define CASS_IMPL_EXPORT __declspec(dllexport) | ||
# else | ||
# define CASS_IMPL_EXPORT __declspec(dllexport) |
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 pretty sure the #else
should be __declspec(dllimport)
. This is an existing typo. Let me do some research.
There was an issue where |
CPP-878 - Correct compile flags for libraries and executable
It fixes crash (or assert failure in Debug configuration) when some .so uses other version (1.1) of rapidjson from /usr/include and overrides some (not all) rapidjson methods compiled in libcassandra.so. This reproduced when connecting to scylladb. Under debugging it looks like rapidjson::Document::StartObject (from /usr/include/rapidjson/...) called with wrong this pointer from GenericParser::ParseObject (from third_party/rapidjson/...), but preprocessor output shows only sources from third_party/rapidjson.