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

Module linkage fixes for shared build #4169

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

kamrann
Copy link
Contributor

@kamrann kamrann commented Sep 20, 2024

Following on from #4153:

  • In regards to constexpr, this was indeed a Clang bug, and anything already marked constexpr should be a non-issue; the problem has been fixed on Clang trunk with potential for it to be backported to the 19.x release.
  • A number of other linker errors specific to mixing #include with import were also being caused by a related Clang bug, also now fixed on trunk.
  • Despite this, a bunch of linker errors still remained when trying to consume the fmt module via a shared library build, due to the last bullet point in the linked issue.

This PR addresses those remaining issues by adding FMT_INLINE/FMT_CONSTEXPR to some member functions defined within the class definition. However, due to the nature of linker issues, problematic cases are only being picked up where a symbol is actually being used from an external dll/exe. Currently, the coverage for that is limited - fmt's own CI is not yet running modules builds I think (or anyway not with FMT_SHARED); the build2 fmt package CI also cannot stress test this as the very latest Clang is not available. Furthermore, I've so far only enabled a subset of the tests in the build2 package.

Fundamentally, I think this addition of inline or constexpr is essentially needed on every single in-class defined member function which is intended for public use (or alternatively FMT_API on the member/class), which is probably a lot more than what I've addressed in these commits and would be quite intrusive. So of course feel free to merge if you like, but it's possible some more thought is warranted in regards to

  • is every public member function in the headers actually intended to be usable directly by clients (this question of course also relates to what should be exported from the module, but for export, doing things in bulk is possible and less intrusive in terms of code changes)
  • is there a preference for exposing those that should be exposed as inline vs FMT_API

…texpr, to avoid missing external symbols when using fmt module with shared build due to modules not defaulting to implicit inline.
NOTE: Looks as if basic_format_args::get(string_view) could probably be made constexpr instead, but sticking with minimal change approach.
@kamrann
Copy link
Contributor Author

kamrann commented Sep 20, 2024

Linter seems to be complaining about formatting that I didn't change.

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.

1 participant