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

Warning C4251: class 'fmt::v8::file' needs to have dll-interface #2796

Closed
dalboris opened this issue Mar 5, 2022 · 5 comments · Fixed by #2797
Closed

Warning C4251: class 'fmt::v8::file' needs to have dll-interface #2796

dalboris opened this issue Mar 5, 2022 · 5 comments · Fixed by #2797

Comments

@dalboris
Copy link
Contributor

dalboris commented Mar 5, 2022

I get the following warning with fmt 8.1.1 (or latest commit) on MSCV 2019 (when compiling as a shared lib):

C:\Users\dalbo\vgc\third\fmt\include\fmt\os.h:414: warning: C4251: 'fmt::v8::ostream::file_': class 'fmt::v8::file' needs to have dll-interface to be used by clients of class 'fmt::v8::ostream'
C:\Users\dalbo\vgc\third\fmt\include\fmt/os.h(287): note: see declaration of 'fmt::v8::file'

The class file indeed isn't declared with FMT_API, although all its non-inline member functions are. The warning goes away if I add FMT_API to the whole class, and remove all the seperate per-function FMT_API.

Was there a good reason for not exporting the whole class, or should I make a PR exporting the whole class?

@dalboris
Copy link
Contributor Author

dalboris commented Mar 5, 2022

Note: I don't have the warning with 7.1.3, but have the warning with 8.0.0, so something seems to have changed between those two versions causing the warning.

@dalboris
Copy link
Contributor Author

dalboris commented Mar 5, 2022

Oh, I see what has changed: it's the class ostream which is exported with FMT_API since 8.0.0.

@dalboris
Copy link
Contributor Author

dalboris commented Mar 5, 2022

This is the commit making the change, which apparently missed adding FMT_API to file too: 13e6529

@vitaut
Copy link
Contributor

vitaut commented Mar 5, 2022

Could you submit a PR to add FMT_API where necessary?

@dalboris
Copy link
Contributor Author

dalboris commented Mar 5, 2022

@vitaut : yes, done!

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 a pull request may close this issue.

2 participants