Feature/complete module #2340

DanielaE · 2021-06-04T16:51:51Z

Add all missing pieces to complete {fmt} as module:

support non-char overloads
support compile-time argument type checking
support compile-time format string compilation

This includes the macro-based API FMT_STRING and FMT_COMPILE by means of a module companion header fmt.h

DanielaE · 2021-06-05T07:13:36Z

include/fmt/format.h

+  using detail::specs_handler;
+  using detail::to_unsigned;
+  using detail::type;
+  using detail::write;


I'm still not sure, if this is really needed by the standard or if this is just an implementation deficit. At least it looks ugly.

From reading "C++ Templates - The Definitive Guide" and the standard, it is clear that the local class is a templated entity. I need further studies of the standard to find an answer to the question: which names within this so-called 'temploid' are bound at which time.

I'd rather prefer to make this 'temploid' a proper class template.

Which local class are you referring to?

struct format_handler within vformat_to in format.h, currently line 2649.

The problem is explored with this stripped-down code example here https://godbolt.org/z/34Ph9dee9. Fortunately I got this compiled with gcc (modules-branch) for the first time just a few minutes ago, so that I have a second opinion on the matter now: gcc doesn't require this 'symbolic-linking' of names into the body of vformat_to and all of my studies of the standard seem to corroborate that. Therefore I consider this an implementation bug in latest msvc. This symbolic linking is the only workaround that works at all. Qualified name lookup doesn't. And only local classes within function templates are affected.

This is acknowledged as bug in msvc by realiable sources familiar with the compiler: name-binding is happening in phase 2 instead of phase 1 in this particular case. In other words: too late to find anything in namespace detail.

Could you add a short comment clarifying that this is a workaround for an msvc bug?

vitaut

Thanks for the PR.

In general looks good but i don't think we should be introducing a new header for macros because FMT_STRING and FMT_COMPILE are effectively obsolete with C++20. The former is redundant and the latter is replaced with a UDL.

vitaut · 2021-06-09T12:54:48Z

include/fmt/compile.h

+FMT_MODULE_EXPORT_BEGIN
+
+// compile-time support
+namespace ct {


Why new namespace?

This is stuff that was in namespace detail before. It must be made visible to be used from importers, f.e. by the ""_cf udl. The compiled_string tag class can be moved into namespace fmt just as well.

Why can't we make detail::compiled_string visible? It's an implementation detail and not supposed to be directly used by anyone, so detail is the correct namespace.

If we make detail::compiled_string visible then the namespace detail becomes implicitly visible, too. This disenfranchises testing from ever checking if anything from that namespace becomes unwittingly visible that was never meant to be visible int the first place. This is the sole reason why I introduced namespace ct: everything in there is the sole minimum of non-public API that needs to be visible. I'd be sad if we'd call namespace detail "open season" to visibility. I was bugging Cameron to fix the current issue in msvc of detail being visible even if nothing in there is currently visible (he checked this with his BMI explorer tool) and he told me that this should be easy to fix. 🤞

I've just finished reworking the PR and peeked here to check for any news. Part of the rework was moving compiled_string to namespace fmt because I got the feeling you were objecting to putting it into ct where other necessarily visible compile-time related stuff is moved to as well. I will push the PR as it is now. Changing it in either direction is a piece of cake and can be done within a few minutes.

Not making detail visible makes sense but ct looks like part of the public API which is undesirable. I suggest renaming ct to something like detail_exported to make it clear that this is an internal API but exported for module purposes.

vitaut · 2021-06-09T12:58:46Z

test/module-test.cc

+  std::wstring w;
+  fmt::format_to(std::back_inserter(w), L"{}", 42);
+  EXPECT_EQ(L"42", w);
+
+  wchar_t wbuffer[4] = {0};
+  fmt::format_to(wbuffer, L"{}", 42);
+  EXPECT_EQ(L"42", std::wstring_view(wbuffer));
+
+  fmt::wmemory_buffer wb;
+  fmt::format_to(wb, L"{}", 42);
+  EXPECT_EQ(L"42", std::wstring_view(wbuffer));


As before, I think this does too much functional testing. I suggest having a single wide format_to call.

These are different overloads in the API. I think this is the minimal set for full coverage.

OK, let's keep them.

vitaut · 2021-06-09T13:00:25Z

test/module-test.cc

+  EXPECT_WRITE(stdout, fmt::print(L"{}µ", 42), as_string(L"42µ"));
+  EXPECT_WRITE(stderr, fmt::print(stderr, L"{}µ", 4.2), as_string(L"4.2µ"));


These can be problematic because IIRC they may fail if the stream is not in wide mode. I suggest only checking that this compiles without actually running, i.e. wrap in if (false).

Ok, will do.

DanielaE · 2021-06-09T15:01:57Z

Even better then. I'm not keen on seeing macros in any API.

FMT_STRING being obsolete is news to me. AFAICS the code paths are different with and without FMT_STRING. And it is used internally too which was giving me quite some head-scratching how to make these internal uses work, not to mention external uses. Cameron was kindly helping me analyzing the problem.

Both macros are listed in the documentation as official part of the API and this is why I felt like providing them.

vitaut · 2021-06-10T12:45:48Z

FMT_STRING will still be supported for pre-C++20 compilers but since modules are C++20 we don't need to export it here.

DanielaE · 2021-06-10T15:19:16Z

BTW, I've removed the block in my local repo that prevents msvc from running the enforce-checks-test: it passes the test with flying colours. The full CI matrix for Windows is green.

vitaut · 2021-06-10T19:50:32Z

include/fmt/compile.h

+FMT_MODULE_EXPORT_BEGIN
+
+// A compile-time string which is compiled into fast formatting code.
+class compiled_string {};


Does it still need to be public if we don't export the FMT_COMPILE macro and only provide a UDL?

Well spotted - thanks!
All related stuff removed.

vitaut · 2021-06-11T13:41:21Z

include/fmt/compile.h


 #if FMT_USE_NONTYPE_TEMPLATE_PARAMETERS
-template <typename Char, size_t N, fixed_string<Char, N> Str>
+template <typename Char, size_t N, fmt::detail_exported::str<Char, N> Str>


I suggest keeping the old name (fixed_string) which is more descriptive.

Right.
My concern is the size of these fmt::v7::detail_exported::fixed_string linker symbols. This is just the common prefix. If you look at real code, the actual linker symbols with the specialized fixed strings become horribly long. May be I can convince Cameron to use a different mangling, may be derived from the base64-encoded SHA1 of the string value.

Anyway, I will change that.

vitaut · 2021-06-11T14:01:20Z

include/fmt/format.h

+// Converts a compile-time string to basic_string_view.
+template <typename Char, size_t N>
+constexpr auto string_to_view(const Char (&s)[N]) -> basic_string_view<Char> {
+  // Remove trailing NUL character if needed. Won't be present if this is used
+  // with a raw character array (i.e. not defined as a string).
+  return {s, N - (std::char_traits<Char>::to_int_type(s[N - 1]) == 0 ? 1 : 0)};
+}
+template <typename Char>
+constexpr auto string_to_view(detail::std_string_view<Char> s)
+    -> basic_string_view<Char> {
+  return {s.data(), s.size()};
+}


These are only used in macros that are not exported so probably should be in the detail namespace, not detail_exported. Also please keep the old names (compile_string_to_view).

This is not correct. These overloads must be visible or xchar.h:231 will fail to compile. And there is no workaround for that.
I will change the name. Even though nothing is compiled here but just a view to the stored data created.

…on in module Make just the necessary parts available for lookup from client context.

vitaut · 2021-06-11T16:39:00Z

Thank you!

DanielaE force-pushed the feature/complete-module branch from 721829d to 929f2ba Compare June 5, 2021 06:55

DanielaE commented Jun 5, 2021

View reviewed changes

DanielaE force-pushed the feature/complete-module branch from 929f2ba to 32af9f9 Compare June 5, 2021 08:21

DanielaE marked this pull request as ready for review June 5, 2021 11:17

vitaut requested changes Jun 9, 2021

View reviewed changes

vitaut reviewed Jun 9, 2021

View reviewed changes

DanielaE force-pushed the feature/complete-module branch from 32af9f9 to 8f45f23 Compare June 10, 2021 15:00

vitaut reviewed Jun 10, 2021

View reviewed changes

DanielaE force-pushed the feature/complete-module branch 2 times, most recently from 669e148 to 60cdb0e Compare June 11, 2021 13:32

vitaut reviewed Jun 11, 2021

View reviewed changes

DanielaE force-pushed the feature/complete-module branch from 60cdb0e to 8e7b4db Compare June 11, 2021 13:58

vitaut reviewed Jun 11, 2021

View reviewed changes

DanielaE added 2 commits June 11, 2021 17:24

Support compile-time strings and compile-time format string compilati…

b63ce15

…on in module Make just the necessary parts available for lookup from client context.

Support non-char overloads (module)

1a351a2

DanielaE force-pushed the feature/complete-module branch from 8e7b4db to 1a351a2 Compare June 11, 2021 15:25

vitaut merged commit 55010a9 into fmtlib:master Jun 11, 2021

DanielaE deleted the feature/complete-module branch June 11, 2021 17:32

		EXPECT_WRITE(stdout, fmt::print(L"{}µ", 42), as_string(L"42µ"));
		EXPECT_WRITE(stderr, fmt::print(stderr, L"{}µ", 4.2), as_string(L"4.2µ"));

Feature/complete module #2340

Feature/complete module #2340

Uh oh!

Conversation

DanielaE commented Jun 4, 2021

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!

vitaut left a comment

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!

vitaut Jun 11, 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!

DanielaE commented Jun 9, 2021

Uh oh!

vitaut commented Jun 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DanielaE commented Jun 10, 2021

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 Jun 11, 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!

vitaut commented Jun 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

vitaut Jun 11, 2021 •

edited

Loading

vitaut commented Jun 10, 2021 •

edited

Loading

DanielaE Jun 11, 2021 •

edited

Loading