Skip to content

Conversation

BestITUserEUW
Copy link
Contributor

This is only a draft for now. This PR addresses #520

@liuzicheng1987 Do you want any changes with the naming or the header file name ?

Apart from this i wanted to add a shared msvc pipeline but i ran into issues with the dll in the test executables not being on path.

Do you by any chance know an easy fix to this ?

Possible solutions i found are putting the build folder on path for all the test exes which i failed to do so.

Create a cmake helper function that copies everything that matches .dll from build folder into the test exes folder but that didn't feel right. (reflectcpp.dll is not enough as this again tries to load dependent dlls)

Copy link

Summary of Changes

Hello @BestITUserEUW, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request is a draft aimed at enabling the reflectcpp library to be built and consumed as a shared library, particularly on MSVC. It introduces the necessary compiler directives and macros to manage symbol visibility, allowing the library's components to be correctly exported and imported when compiled into a DLL. This change is crucial for users who wish to integrate reflectcpp into projects that require shared library architectures.

Highlights

  • Shared Library Support: Introduced a new RFL_API macro to properly handle symbol export and import for shared libraries (DLLs) across different platforms, specifically addressing MSVC.
  • CMake Configuration: Updated CMakeLists.txt to conditionally define RFL_BUILD_SHARED when building the library as a shared object, and YYJSON_EXPORTS/YYJSON_IMPORTS for JSON functionality, ensuring correct compilation for shared builds.
  • API Visibility: Modified numerous header files across various serialization formats (e.g., Avro, BSON, Cap'n Proto, JSON, TOML, XML) to include the new common.hpp and apply the RFL_API macro to classes and functions, making them correctly visible when reflectcpp is built as a shared library.
Ignored Files
  • Ignored by pattern: .github/workflows/** (1)
    • .github/workflows/windows.yaml
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces the necessary changes to support building reflectcpp as a shared library, particularly for MSVC on Windows. This is achieved by adding a common.hpp header that defines the RFL_API macro for symbol visibility, and applying this macro across the public API of the library. The CMake build system is also updated to handle the shared library build configuration. The changes are extensive but applied consistently. My review includes a critical fix for the CMake configuration to correctly handle symbol visibility for the bundled yyjson dependency, and a minor style fix.

Regarding your question about handling DLLs for test executables, you've run into a common issue on Windows. A clean way to solve this in CMake is to have all runtime artifacts (executables and DLLs) placed in the same directory. You can achieve this by setting the CMAKE_RUNTIME_OUTPUT_DIRECTORY variable early in your CMakeLists.txt. For example:

# Place all executables and DLLs in the same directory
set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/bin)

This way, the test executables will be in the same folder as reflectcpp.dll and all its dependency DLLs, and they will be found automatically at runtime. This is generally preferable to copying files around with custom commands.

CMakeLists.txt Outdated
Comment on lines 182 to 189
if(REFLECTCPP_JSON)
target_compile_definitions(reflectcpp
PRIVATE
YYJSON_EXPORTS
PUBLIC
YYJSON_IMPORTS
)
endif()

Choose a reason for hiding this comment

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

high

The current logic for defining YYJSON_EXPORTS and YYJSON_IMPORTS has a few issues:

  1. Conflicting Definitions: By using PUBLIC for YYJSON_IMPORTS, both YYJSON_EXPORTS (from PRIVATE) and YYJSON_IMPORTS will be defined when building reflectcpp. This is usually incorrect and can lead to compilation errors, as headers typically use an #ifdef ... #elif ... #else structure for symbol import/export.
  2. Static Builds: These definitions are only required for shared library builds. They are unnecessary for static builds and might cause issues.
  3. External Dependencies: This should only apply when using the bundled yyjson dependency. If an external yyjson is used, it should manage its own symbol visibility.

To address this, I suggest making these definitions conditional on it being a shared build that uses the bundled dependency, and using INTERFACE to propagate YYJSON_IMPORTS to consumers without defining it for reflectcpp itself. The condition should also use _REFLECTCPP_NEEDS_JSON_IMPL to cover all formats that use yyjson.

if(REFLECTCPP_BUILD_SHARED AND _REFLECTCPP_NEEDS_JSON_IMPL AND REFLECTCPP_USE_BUNDLED_DEPENDENCIES)
    target_compile_definitions(reflectcpp
        PRIVATE
            YYJSON_EXPORTS
        INTERFACE
            YYJSON_IMPORTS
    )
endif()

#define RFL_API
#endif

#endif No newline at end of file

Choose a reason for hiding this comment

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

medium

It's a common convention to end files with a newline character. Some tools might issue a warning if it's missing.

Suggested change
#endif
#endif

@BestITUserEUW BestITUserEUW marked this pull request as draft October 17, 2025 13:17
@BestITUserEUW
Copy link
Contributor Author

BestITUserEUW commented Oct 17, 2025

@liuzicheng1987 Is there a specific reason that only avro static library is linked when using vcpkg ?

Linking a static library into a shared one complicates things.

Avro issue could be fixed by linking public to avro and all it's dependants as we are linking manually + using whole archive like the following:

target_link_libraries(reflectcpp 
                    PUBLIC 
                        $<LINK_LIBRARY:WHOLE_ARCHIVE,${_AVRO_STATIC_LIB}> 
                        "${VCPKG_INSTALLED_DIR}/${VCPKG_TARGET_TRIPLET}/lib/snappy${CMAKE_STATIC_LIBRARY_SUFFIX}" 
                        "${VCPKG_INSTALLED_DIR}/${VCPKG_TARGET_TRIPLET}/lib/lzma${CMAKE_STATIC_LIBRARY_SUFFIX}" 
                        "${VCPKG_INSTALLED_DIR}/${VCPKG_TARGET_TRIPLET}/lib/jansson${CMAKE_STATIC_LIBRARY_SUFFIX}"
)

This essentially means that the consumer of this library also has to link against these.

or providing an abstraction layer for avro and use it solely in compilation units. So avro does not need to be linked against by consumers

or just get a shared build of it

@BestITUserEUW BestITUserEUW marked this pull request as ready for review October 18, 2025 16:34
@BestITUserEUW
Copy link
Contributor Author

BestITUserEUW commented Oct 18, 2025

Sources for changes i applied:

Avro dependency list: avro-c
Custom copy command from cmake: Target Runtime Dlls
Link whole archive: Cmake Whole Arhive

@BestITUserEUW
Copy link
Contributor Author

@liuzicheng1987 Ready for review. In the shared tests you can see that .lib file is generated correctly and also verifies that the linkage for all implementations work and they are runnable.

 Generating Code...
     Creating library D:/a/reflect-cpp/reflect-cpp/build/Release/reflectcpp.lib and object D:/a/reflect-cpp/reflect-cpp/build/Release/reflectcpp.exp
  reflectcpp.vcxproj -> D:\a\reflect-cpp\reflect-cpp\build\Release\reflectcpp.dll

As there was no answer to my previous question i took the most non-intrusive solution for avro. Make it public link the whole archive and also it's dependants.

Sources for my actions can be found in the comment above

@liuzicheng1987
Copy link
Contributor

@BestITUserEUW , thank you so much for your effort!

@liuzicheng1987 liuzicheng1987 merged commit 83951fd into getml:main Oct 19, 2025
157 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants