Provide a module interface unit by DanielaE · Pull Request #2240 · fmtlib/fmt

DanielaE · 2021-04-17T09:22:50Z

To compile {fmt} as named module, there must be at least the source provided with the (primary) module interface.

I chose the most elegant and probably the most desirable implementation strategy: a single-file module with a private module partition. This has these benefits:

easiest to use
the existing headers require no changes other than providing macro hooks for the exported entities (if implemented cleanly)
all macros are available during compilation and require no further attention
the existing source parts all go into the non-exported, unreachable private module partition, which is basically a module implementation unit that's directly fused to end of the module interface unit
there are only 2 artefacts created:
1. the object file
2. the built module interface (BMI)

The macro-based interfaces FMT_STRING and FMT_COMPILE are not yet implemented. Macros cannot get exported from modules.

The inclusion of the 'os' part is optional an can be configured from the command line at build time.
The module name can be configured from the command line at build time. By default it is 'fmt'.
The module may be configured and provided as a static or dynamic-load lib as before.

This requires a sufficiently complete and bug-free implementation of C++20 modules to compile!

At present, msvc 16.10-pre2 requires a workaround for successful compilation (which is not included). 16.10-pre3 is reportedly fine according the the compiler devs.
I have no experience how gcc and clang fare. Let's give them a serious drill!

@vitaut : this is how I do it n my own repo. Let's discuss this.

vitaut

Thanks a lot for the PR! Looks good, just a few minor comments inline.

vitaut · 2021-04-22T15:15:53Z

+  #endif
+#endif
+
+export module FMT_MODULE_NAME;


I don't think the module name should be configurable. Let's use fmt here.

Wonderful, we can agree on a name.

vitaut · 2021-04-22T15:16:08Z

+#define FMT_MODULE_EXPORT_BEGIN export {
+#define FMT_MODULE_EXPORT_END }
+
+#define FMT_USE_NONTYPE_TEMPLATE_PARAMETERS 1


What is this for?

C++20 modules unsurprisingly require C++20 which includes NTTPs. compile.h enables the use of NTTPs in a non-standard way that excludes compilers like msvc that support this feature. My current code to support FMT_STRING and FMT_COMPILE relies on this feature. So I enable it by overriding the macro until compile.h gets updated to the reality.

Looks like we should use this as a fix - foonathan/lexy#15

It should be fixed with #2247, but I need help with this PR, some strange MSVC errors...

This FMT_STRING thing is bothering me in a different context, too. At present fmt::to_wstring() cannot be invoked from client code because identifiers from the expansion of FMT_STRING cannot be looked-up from the body of instantiations of to_wstring. But that should be adressed in a different PR.

C++20 modules unsurprisingly require C++20 which includes NTTPs.

While this is true, modules and NTTP are independent features so presence of one doesn't imply presence of the other. I suggest removing this define from the current PR. It can be set when building {fmt}.

Ok with me.

vitaut · 2021-04-22T15:17:28Z

+#include "fmt/ostream.h"
+#include "fmt/ranges.h"


These two should be moved to separate modules because in many cases they should be disabled / explicitly opted in.

ostream.h is included through printf.h anyway.
ranges.h can be implemented in a different module if it neither requires any macro from other parts of {fmt} nor any non-exported identifier. Not to mention that teensy tiny modules are not recommended afaik.
What do you think about conditionally including these parts of {fmt} by macro definitions from the command line like it is already proposed for os.h? So users of the module can decide on their own if they want this or not.

The currently existing dependency levels are as such:

1
core.h

2
args.h -> core.h
format.h -> core.h

3
compile.h -> format.h
ostream.h -> format.h
ranges.h -> format.h
color.h -> format.h
locale.h -> format.h
os.h -> format.h
format-inl.h -> format.h

4
printf.h -> ostream.h
chrono.h -> format.h locale.h

Not that I have a say in this, but why split it up and/or have conditional inclusion (this also applies to OS)? In principle, it shouldn't matter how much stuff is included in the module and it's not like fmt has no/few standard library dependencies without them.

I'm not sure if I get your point here. You are right about your comment on the amount of stuff included in a module, but why do you mention the standard library? Do you propose not to carve out anything from inclusion into the module interface? If so we are on the same page. So far I've excluded os.h for the simple fact that it doesn't matter: nothing is exported from there because there doesn't seem to exist a documented interface that hints on which declarations to export. Additional exports can be added in a subsequent PR by tagging them accordingly. IIUC, Victor's concerns are about template specializations they bring into the TUs that import {fmt}. I have hardly any clue about ranges.h in this regard but I cannot really see a problem with ostream.h.

It must be split because ostream and ranges provide formatter specialization that should not be available by default.

Which then rules out printf.h, too. Oh well...

printf.h dependency on ostream.h is accidental and will be removed by b9ab5c8.

DanielaE · 2021-04-23T17:22:50Z

So I will proceed this way:

make the module name fixed fmt
keep FMT_USE_NONTYPE_TEMPLATE_PARAMETERS for the near future
remove os.h, ostream.h, and ranges.h.
remove system headers that are no longer nominated by the remaining parts of {fmt}

Additional changes like already mentioned need to be subject of PRs against regular {fmt}.

vitaut · 2021-04-23T18:04:10Z

remove os.h

I think it's OK to keep os.h. It's mostly experimental but not harmful.

DanielaE · 2021-04-23T18:06:23Z

Looks like a plan to me. Hold tight...

vitaut · 2021-04-24T13:40:06Z

Merged, thank you!

vitaut · 2021-04-25T15:49:08Z

I am getting the following error when building the module with gcc:

$ g++-11 src/fmt.cc -I include -std=c++20 -fmodules-ts
In file included from src/fmt.cc:75:
include/fmt/format.h:3898:66: error: conflicting exporting declaration ‘template<class Char, typename std::enable_if<(! std::is_same<Char, char>::value), int>::type <anonymous> > std::__cxx11::basic_string<_CharT> fmt::v7::detail::vformat(fmt::v7::basic_string_view<Char>, fmt::v7::basic_format_args<fmt::v7::basic_format_context<fmt::v7::detail::buffer_appender<typename fmt::v7::type_identity<T>::type>, typename fmt::v7::type_identity<T>::type> >)’
 3898 |     basic_format_args<buffer_context<type_identity_t<Char>>> args) {
      |                                                                  ^
In file included from include/fmt/format.h:46,
                 from src/fmt.cc:75:
include/fmt/core.h:1800:25: note: previous declaration ‘template<class Char, typename std::enable_if<(! std::is_same<Char, char>::value), int>::type <anonymous> > std::__cxx11::basic_string<_CharT> fmt::v7::detail::vformat(fmt::v7::basic_string_view<Char>, fmt::v7::basic_format_args<fmt::v7::basic_format_context<fmt::v7::detail::buffer_appender<typename fmt::v7::type_identity<T>::type>, typename fmt::v7::type_identity<T>::type> >)’ here
 1800 | std::basic_string<Char> vformat(
      |                         ^~~~~~~

Any ideas?

vitaut · 2021-04-25T15:52:41Z

I think the problem is that the declaration is exported twice.

DanielaE · 2021-04-25T18:06:55Z

Ok, if so, the problem should be easy to resolve by exporting it only from the location where the name is intoduced first. That's most likely in core.h. MSVC didn't complain and I don't have any other compiler available to test with.

DanielaE · 2021-04-25T18:12:28Z

Strange. The declaration in core.h is in namespace detail, the one in format.h is in namespace fmt. I hope neither file is changed too much, my clone isn't up-to-date.

vitaut requested changes Apr 22, 2021

View reviewed changes

alexezeder mentioned this pull request Apr 22, 2021

Use proper check for non-type template parameters #2247

Merged

Export replacement type_traits, too

05a83e8

DanielaE force-pushed the feature/named-module branch from 9d29eff to ab54a18 Compare April 24, 2021 08:26

DanielaE added 2 commits April 24, 2021 10:41

Export printf-related contexts from printf.h

4628394

Add module interface unit

99bb864

DanielaE force-pushed the feature/named-module branch from ab54a18 to 99bb864 Compare April 24, 2021 08:42

vitaut merged commit dacd135 into fmtlib:master Apr 24, 2021

alexezeder mentioned this pull request Apr 28, 2021

Support 'to_wstring(T)' in module. #2259

Closed

		#include "fmt/ostream.h"
		#include "fmt/ranges.h"

Uh oh!

Conversation

DanielaE commented Apr 17, 2021

Uh oh!

vitaut left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanielaE Apr 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanielaE commented Apr 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vitaut commented Apr 23, 2021

Uh oh!

DanielaE commented Apr 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vitaut commented Apr 24, 2021

Uh oh!

vitaut commented Apr 25, 2021

Uh oh!

vitaut commented Apr 25, 2021

Uh oh!

DanielaE commented Apr 25, 2021

Uh oh!

DanielaE commented Apr 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

DanielaE Apr 23, 2021 •

edited

Loading

DanielaE commented Apr 23, 2021 •

edited

Loading

DanielaE commented Apr 23, 2021 •

edited

Loading