The dll.h is made static again (revert commit 0733aeb4) #1064
Conversation
…beder#984)" (jbeder#1038)" (jbeder#1045)" This reverts commit 0733aeb.
New dll.h is assembly from cmake-generation for GCC, MSVC, MIGW
| # ifndef YAML_CPP_NO_EXPORT | ||
| # define YAML_CPP_NO_EXPORT | ||
| # endif | ||
| # else /* No _MSC_VER */ |
There was a problem hiding this comment.
What cases (compilers/operating systems) does this else case catch? What's the reason for the differing definition?
There was a problem hiding this comment.
As far as I know, in Windows all functions are hidden by default for export. You must explicitly use the __declspec(dllexport) directive to export functions from the library. On the contrary, in Linux all functions by default are exports. Sometimes in Linux (often for reasons of project build speed) there is a need to hide some of the functions. To do this, using the directive __attribute__((("hidden")). If in the future will be need to hide some of the functions for assembly on Linux/Unix platforms, this macro will be useful. Now it's just a formal record.
There was a problem hiding this comment.
@jbeder it's much the same as the suggested form from gcc https://gcc.gnu.org/wiki/Visibility
| else() | ||
| set(yaml-cpp-type STATIC) | ||
| set(yaml-cpp-label-postfix "static") | ||
| add_definitions(-DYAML_CPP_STATIC_DEFINE) |
There was a problem hiding this comment.
It seems more natural to me to add a definition for the shared lib, which is the unusual case. (I don't feel super strongly, but since it looks like it doesn't matter one way or another, I thought I'd say something.)
There was a problem hiding this comment.
I am not a specialist at CMake, so this change is made by virtue of my little knowledge in this assembly system. I guess you can change this section as you see fit.
There was a problem hiding this comment.
OK, I guess it really doesn't matter much. I'll merge the code now and any improvements can be made after the fact.
| // (definition created by CMake or defined manually) | ||
|
|
||
| #ifdef YAML_CPP_STATIC_DEFINE | ||
| # define YAML_CPP_API |
There was a problem hiding this comment.
Do you need to define these symbols at all in the static case?
There was a problem hiding this comment.
Yes, for a static assembly, the YAML_CPP_STATIC_DEFINE must be defined in the yaml-cpp library, as well as in the target application.
For the static case, the macros YAML_CPP_API and YAML_CPP_NO_EXPORT must be dummy.
There was a problem hiding this comment.
What I mean is, in the static case, do you need to define YAML_CPP_API at all?
There was a problem hiding this comment.
M-m-m...perhaps I don't quite understand the question (translation difficulties). But still I'll try to answer: theoretically, for the variant of static build the definition YAML_CPP_API is not needed, but from a practical point of view, YAML_CPP_API must be present - otherwise the project will not build
hkarel
changed the base branch from
revert-1045-hidden-visibility-again
to
master
I relocated PR to master |
it's possible you might have to make a new PR if you were missing the github actions file(s) originally, although @jbeder can probably run it manually |
I tried creating a new PR (copy of this PR). It didn't work, I was redirected to this page. Apparently github keeps track of PR copies. |
|
it normally lets you create a new one but shows you the existing one in case you just want that one |
| else() | ||
| set(yaml-cpp-type STATIC) | ||
| set(yaml-cpp-label-postfix "static") | ||
| add_definitions(-DYAML_CPP_STATIC_DEFINE) |
There was a problem hiding this comment.
OK, I guess it really doesn't matter much. I'll merge the code now and any improvements can be made after the fact.
…beder#1064) Partial revert of "Revert "Revert "Hide most of non-public symbols by default (jbeder#984)" (jbeder#1038)" (jbeder#1045)" This reverts commit 0733aeb.
New dll.h is assembly from cmake-generation for GCC, MSVC, MIGW