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.

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

Fix warning C4251: class fmt::v8::file needs to have dll-interface #2797

Merged
merged 2 commits into from
Mar 7, 2022

Conversation

dalboris
Copy link
Contributor

@dalboris dalboris commented Mar 5, 2022

These changes fix #2796 by exporting the whole class fmt::v8::file instead of exporting all its individual member functions.

I agree that my contributions are licensed under the {fmt} license, and agree to future changes to the licensing.

@vitaut
Copy link
Contributor

vitaut commented Mar 7, 2022

There are some link errors: https://github.com/fmtlib/fmt/runs/5442603541

@dalboris
Copy link
Contributor Author

dalboris commented Mar 7, 2022

@vitaut Looking into it

@dalboris
Copy link
Contributor Author

dalboris commented Mar 7, 2022

The problem was that fmt was missing in this line in test/CMakeLists :

target_link_libraries(test-main gtest fmt)

Without the fmt dependency, the test-main target didn't inherit the FMT_SHARED compile definition defined L272 of fmt/CMakeLists:

target_compile_definitions(fmt PRIVATE FMT_EXPORT INTERFACE FMT_SHARED)

Therefore, when compiling test-main, the macro FMT_API resolved to nothing, so test-main didn't know it had to import existing symbols (such as fmt::file::file() and others) and instead re-defined these symbols.

Later, when compiling a test (for exemple, os-test.cc), the test does depend on fmt, so it inherits FMT_SHARED, so FMT_API resolves to __declspec(dllimport), so it correctly imports the symbols from the fmt shared lib, which conflicts with the redefined symbols in test-main.

@dalboris
Copy link
Contributor Author

dalboris commented Mar 7, 2022

Shout out to @PixelRick who found the problem after a lot of head scratching together.

@vitaut vitaut merged commit e3d688e into fmtlib:master Mar 7, 2022
@vitaut
Copy link
Contributor

vitaut commented Mar 7, 2022

Thank you

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.

Warning C4251: class 'fmt::v8::file' needs to have dll-interface
2 participants