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

Use native c++ module support from CMake #3991

Merged
merged 4 commits into from
Jun 6, 2024
Merged

Conversation

yujincheng08
Copy link
Contributor

also fix some clang compilation issues when using c++ modules

close #3990

Copy link
Contributor

@vitaut vitaut left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Overall looks good, just two minor comments inline.

CMakeLists.txt Outdated Show resolved Hide resolved
@@ -4285,6 +4288,8 @@ extern template FMT_API auto decimal_point_impl(locale_ref) -> char;
extern template FMT_API auto decimal_point_impl(locale_ref) -> wchar_t;
#endif // FMT_HEADER_ONLY

FMT_END_EXPORT
Copy link
Contributor

Choose a reason for hiding this comment

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

Why exclude native_formatter<T, Char, TYPE>::format from export? If you get an error please post it here for future reference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The error is:

In file included from fmt/src/fmt.cc:99:
In file included from fmt/include/fmt/args.h:17:
In file included from fmt/include/fmt/args.h:17:
fmt/include/fmt/format.h:4293:64: error: cannot export 'format' as it is not at namespace scope
 4293 | FMT_CONSTEXPR FMT_INLINE auto native_formatter<T, Char, TYPE>::format(
      |                               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^
fmt/include/fmt/format.h:4295:7: error: invalid use of non-static data member 'specs_'
fmt/include/fmt/format.h:4295:7: error: 'specs_' is a private member of 'native_formatter<T, Char, TYPE>'
fmt/include/fmt/base.h:2796:30: note: declared private here
 4295 |   if (specs_.width_ref.kind == arg_id_kind::none &&
 2796 |   dynamic_format_specs<Char> specs_;
      |       ^~~~~~
      |                              ^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe useful: llvm/llvm-project#61890

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So if you wanna export native_formatter<T, Char, TYPE>::format you should export native_formatter<T, Char, TYPE> instead. I tried and it compiled. If you think it okay I can commit it.

@yujincheng08 yujincheng08 requested a review from vitaut June 6, 2024 02:14
@vitaut vitaut merged commit 5c445bc into fmtlib:master Jun 6, 2024
42 checks passed
@vitaut
Copy link
Contributor

vitaut commented Jun 6, 2024

Merged, thanks!

@yujincheng08 yujincheng08 deleted the modules branch June 6, 2024 05:29
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.

Use native c++ module support from CMake
2 participants