Added possibility to compile this library as a static or dynamic library - #324
Added possibility to compile this library as a static or dynamic library#324atvise wants to merge 1 commit into
Conversation
atvise
commented
Apr 14, 2017
atvise
commented
- visibility.h/Makefile - Added possibility to compile this library as a static or dynamic library
- value.h - Fixed friend declarations because the must have also the PHPCPP_EXPORT declaration (C2375 redefinition; different linkage)
- streams.h - Fixed extern thread_local stream definition because the can't have PHPCPP_EXPORT declaration (C2492 'deprecated': data with thread storage duration may not have dll interface)
- visibility.h/Makefile - Added possibility to compile this library as a static or dynamic library - value.h - Fixed friend declarations because the must have also the PHPCPP_EXPORT declaration (C2375 redefinition; different linkage) - streams.h - Fixed extern thread_local stream definition because the can't have PHPCPP_EXPORT declaration (C2492 'deprecated': data with thread storage duration may not have dll interface)
| friend Value constant(const char *name, size_t size); | ||
| friend bool define(const char *name, size_t size, const Value &value); | ||
| friend PHPCPP_EXPORT Value constant(const char *name, size_t size); | ||
| friend PHPCPP_EXPORT bool define(const char *name, size_t size, const Value &value); |
There was a problem hiding this comment.
This and other items from the file should be a separate PR. The issue will show up with any build with the strict visibility flags, fe on Fedora.
Thanks.
| extern thread_local std::ostream error; | ||
| extern thread_local std::ostream notice; | ||
| extern thread_local std::ostream warning; | ||
| extern thread_local std::ostream deprecated; |
There was a problem hiding this comment.
IMO, this change is wrong. It needs a profound solution for non GNU compatible compiler. Basically, it still needs a DSO export, but should encapsulate thread safe pieces. I'd allow myself also to reference #90 (comment) . Anyway, it doesn't belong to the PR about static enablement, as it would be controlled by the header where PHPCPP_EXPORT would evaluate to empty already. Right now, the DLL build is simply broken, anyway.
Thanks.
| #else /* build static library */ | ||
| #define PHPCPP_EXPORT | ||
| #endif | ||
| #define DLL_EXPORT |
There was a problem hiding this comment.
Why does it need two different specifiers? Either PHPCPP_EXPORT targets DLL or static build.
Thanks.
| #else /* build static library */ | ||
| #define PHPCPP_EXPORT | ||
| #endif | ||
| #define DLL_EXPORT |
There was a problem hiding this comment.
This piece seems wrong. If the attribute is supported, DLL_EXPORT should be same as PHPCPP_EXPORT.
In general I see what you're striving to achieve, but it is not going to work well. An extension should define its own API inside it, using same specifier will still require additional compiler flags. Fe if there's a static phpcpp lib, get_module() inside the extension will still have to be DLL exported.
Thanks.
2 participants
