-
Notifications
You must be signed in to change notification settings - Fork 4.1k
THRIFT-5898: Replace global variable with a singleton getter #3226
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
base: master
Are you sure you want to change the base?
Conversation
|
@CJCombrink I just wanted to thank you for taking care of that issue! |
|
I see the AppVeyor build now fails for something else: I will need to look into this as well |
|
Applied the comment from here: #1714 (comment) |
f8d0f5c to
265ec23
Compare
emmenlau
left a comment
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 a good change.
| PLATFORM: x64 | ||
| CONFIGURATION: Release | ||
| BUILD_SHARED_LIBS: OFF | ||
| BUILD_SHARED_LIBS: ON |
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 this be in a separate PR, to add shared library build? This PR here is 99% only about replacing GlobalOutput, which is useful to me independent of shared libraries.
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.
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.
To follow up on mail conversation:
The CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS does not export global symbols according to the cmake docs
However, there are some cases when compiling code in a consumer may require explicit dllimport markup:
- Global data symbols must be explicitly marked with __declspec(dllimport) in order to link to data in the .dll.
02f8707 to
1471a68
Compare
See the comment made in here where it was suggested but never done: apache#1714 (comment)
1471a68 to
585f471
Compare
Replace the global
GlobalOutputvariable with a static method call to get a singleton instance, preventing the need to export the symbol.This issue also blocks conan from adding 0.22.0 (see conan-io/conan-center-index#27382)
GlobalOutputvariable[skip ci]anywhere in the commit message to free up build resources.