Skip to content
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.

Sign up for GitHub

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

Jump to bottom

The dll.h is made static again (revert commit 0733aeb4) #1064

Merged
merged 3 commits into from
Nov 23, 2021
Merged

The dll.h is made static again (revert commit 0733aeb4) #1064

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
317d6d3
Revert "Revert "Revert "Hide most of non-public symbols by default (#…
jbeder Nov 19, 2021
e1e6344
Add back missing YAML_CPP_API.
jbeder Nov 19, 2021
6323e59
The dll.h is made static again (revert commit 0733aeb4)
hkarel Nov 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 4 additions & 17 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,6 @@ include(CMakeDependentOption)
include(CheckCXXCompilerFlag)
include(GNUInstallDirs)
include(CTest)
include(GenerateExportHeader)

set(CMAKE_C_VISIBILITY_PRESET hidden)
set(CMAKE_CXX_VISIBILITY_PRESET hidden)
set(CMAKE_VISIBILITY_INLINES_HIDDEN 1)

find_program(YAML_CPP_CLANG_FORMAT_EXE NAMES clang-format)

Expand All @@ -36,11 +31,13 @@ cmake_dependent_option(YAML_MSVC_SHARED_RT
"MSVC: Build yaml-cpp with shared runtime libs (/MD)" ON
"MSVC" OFF)

set(yaml-cpp-type STATIC)
set(yaml-cpp-label-postfix "static")
if (YAML_BUILD_SHARED_LIBS)
set(yaml-cpp-type SHARED)
set(yaml-cpp-label-postfix "shared")
else()
set(yaml-cpp-type STATIC)
set(yaml-cpp-label-postfix "static")
add_definitions(-DYAML_CPP_STATIC_DEFINE)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I guess it really doesn't matter much. I'll merge the code now and any improvements can be made after the fact.

endif()

set(build-shared $<BOOL:${YAML_BUILD_SHARED_LIBS}>)
Expand Down Expand Up @@ -86,7 +83,6 @@ set_property(TARGET yaml-cpp
target_include_directories(yaml-cpp
PUBLIC
$<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/include>
$<BUILD_INTERFACE:${PROJECT_BINARY_DIR}/include>
$<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>
PRIVATE
$<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/src>)
Expand Down Expand Up @@ -142,12 +138,6 @@ write_basic_package_version_file(
"${PROJECT_BINARY_DIR}/yaml-cpp-config-version.cmake"
COMPATIBILITY AnyNewerVersion)

generate_export_header(yaml-cpp
BASE_NAME YAML_CPP
EXPORT_FILE_NAME "${PROJECT_BINARY_DIR}/include/yaml-cpp/dll.h"
EXPORT_MACRO_NAME YAML_CPP_API
)

configure_file(yaml-cpp.pc.in yaml-cpp.pc @ONLY)

if (YAML_CPP_INSTALL)
Expand All @@ -157,9 +147,6 @@ if (YAML_CPP_INSTALL)
LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}
ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR})
install(DIRECTORY ${PROJECT_SOURCE_DIR}/include/
DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}
FILES_MATCHING PATTERN "*.h")
install(DIRECTORY ${PROJECT_BINARY_DIR}/include/
DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}
FILES_MATCHING PATTERN "*.h")
install(EXPORT yaml-cpp-targets
Expand Down
61 changes: 61 additions & 0 deletions include/yaml-cpp/dll.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
#ifndef DLL_H_62B23520_7C8E_11DE_8A39_0800200C9A66
#define DLL_H_62B23520_7C8E_11DE_8A39_0800200C9A66

// Definition YAML_CPP_STATIC_DEFINE using to building YAML-CPP as static
// library (definition created by CMake or defined manually)

// Definition yaml_cpp_EXPORTS using to building YAML-CPP as dll/so library
// (definition created by CMake or defined manually)

#ifdef YAML_CPP_STATIC_DEFINE
# define YAML_CPP_API
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need to define these symbols at all in the static case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I mean is, in the static case, do you need to define YAML_CPP_API at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

# define YAML_CPP_NO_EXPORT
#else
# if defined(_MSC_VER) || defined(__MINGW32__) || defined(__MINGW64__)
# ifndef YAML_CPP_API
# ifdef yaml_cpp_EXPORTS
/* We are building this library */
# pragma message( "Defining YAML_CPP_API for DLL export" )
# define YAML_CPP_API __declspec(dllexport)
# else
/* We are using this library */
# pragma message( "Defining YAML_CPP_API for DLL import" )
# define YAML_CPP_API __declspec(dllimport)
# endif
# endif
# ifndef YAML_CPP_NO_EXPORT
# define YAML_CPP_NO_EXPORT
# endif
# else /* No _MSC_VER */
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What cases (compilers/operating systems) does this else case catch? What's the reason for the differing definition?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jbeder it's much the same as the suggested form from gcc https://gcc.gnu.org/wiki/Visibility

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, sounds good to me.

# ifndef YAML_CPP_API
# ifdef yaml_cpp_EXPORTS
/* We are building this library */
# define YAML_CPP_API __attribute__((visibility("default")))
# else
/* We are using this library */
# define YAML_CPP_API __attribute__((visibility("default")))
# endif
# endif
# ifndef YAML_CPP_NO_EXPORT
# define YAML_CPP_NO_EXPORT __attribute__((visibility("hidden")))
# endif
# endif /* _MSC_VER */
#endif /* YAML_CPP_STATIC_DEFINE */

#ifndef YAML_CPP_DEPRECATED
# ifdef _MSC_VER
# define YAML_CPP_DEPRECATED __declspec(deprecated)
# else
# define YAML_CPP_DEPRECATED __attribute__ ((__deprecated__))
# endif
#endif

#ifndef YAML_CPP_DEPRECATED_EXPORT
# define YAML_CPP_DEPRECATED_EXPORT YAML_CPP_API YAML_CPP_DEPRECATED
#endif

#ifndef YAML_CPP_DEPRECATED_NO_EXPORT
# define YAML_CPP_DEPRECATED_NO_EXPORT YAML_CPP_NO_EXPORT YAML_CPP_DEPRECATED
#endif

#endif /* DLL_H_62B23520_7C8E_11DE_8A39_0800200C9A66 */