Hide most of non-public symbols by default #984
Conversation
The internal header node/detail/node.h is included by public headers; YAML::detail::node is implemented in the header itself, and thus it gets inlined... except for its static m_amount class member, which is instantiated in the library only. Right now all the symbols of yaml-cpp are exported (nothing is hidden), so the linker will find node::m_amount in the yaml-cpp library. As solution/workaround, explicitly export YAML::detail::node::m_amount.
Make use of the GenerateExportHeader CMake module to generate the dll.h header with export macros. While the produced dll.h is different, the result should be the same, i.e. nothing changes for yaml-cpp or its users.
Hide all the symbols that are not explicitly exported with YAML_CPP_API. This way the ABI will be way smaller, and only actually exposing the public classes/functions.
fb8bee9 to
5fe6157
Compare
|
Hi @jbeder! Would it be possible to take a look at this? Thanks in advance! |
| @@ -13,7 +13,7 @@ | |||
|
|
|||
| namespace YAML { | |||
| namespace detail { | |||
| std::atomic<size_t> node::m_amount{0}; | |||
| YAML_CPP_API std::atomic<size_t> node::m_amount{0}; | |||
There was a problem hiding this comment.
Isn't same YAML_CPP_API should be in header too?
There was a problem hiding this comment.
No, in the definition is enough, for this internal symbol.
|
FYI this broke the build (see #1036). I reverted this PR; once you've determined the cause and fix, please send an updated PR and I'll approve. Ideally also please add a test that confirms the fix. |
While I understand the concern, it'd have been nice to at least give me a change to look at it before the so quick revert... |
|
Ah, I understand there are different philosophies here, and so I apologize for not communicating the rationale. I'm of the school of thought "rollback first, as questions later" to get the build to a stable state as quickly as possible. Unless absolutely necessary, I prefer rollbacks to fix-forwards. You can Google "rollback vs. fix forward" to see a litany of arguments for or against :) |
|
Sure, I know and understand that. However, it still does not feel nice that the PR took 5 months to even get any action (which got after my explicit mention), and it got ~1 day with no question asked to be reverted. Even worse when the bug report mentioning a problem is so generic that, at least to me, is barely actionable at all. |
|
Yeah, I agree that bug report wasn't great. I'm sorry about the 5 month
delay, this is a side-side-side project for me so my time is bursty. When
you pinged the PR, it looked simple, and then when I saw the bug report, I
worried that, geez, I've ignored the project for months and now I approve a
PR that broke it, so I rolled back in an abundance of caution.
…On Tue, Sep 28, 2021 at 2:01 PM Pino Toscano ***@***.***> wrote:
Sure, I know and understand that. However, it still does not feel nice
that the PR took 5 months to even get any action (which got after my
explicit mention), and it got ~1 day with no question asked to be reverted.
Even worse when the bug report mentioning a problem is *so generic* that,
at least to me, is barely actionable at all.
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#984 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAICUBVQPS2MQKXVZC6SMFLUEIGGZANCNFSM43D7PB3A>
.
|
…" (jbeder#1038)" The issue jbeder#1038 is very generic, and it does not provide any detail on what the actual problems are. Since the hidden visibility changes were correct, revert their revertion to make them available again. This reverts commit 0d9dbcf.
|
before this PR, anyone could take this code and use basically any tooling and build a usable library, but now you have to use cmake or dll.h won't exist. I would really not like to have to use cmake. |
See my answer: #1046 (comment) Just because "it worked" does not mean it was doable "for free", you had (and still have) to do a lot of logic on your own, and this is one bit of logic more done by the build system instead of rolling an own (and limited/buggy) solution. |
This reverts commit da1c8d3.
…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.
Make use of CMake features to generate
dll.hwith export macros, and to hide all the symbols not explicitly marked for exporting.This PR provides a more complete implementation of #644 and #958.