fmtlib / fmt

A modern formatting library
https://fmt.dev
Other
20.56k stars 2.46k forks source link

MSVC errors when importing both fmt and std modules #3921

Closed matt77hias closed 5 months ago

matt77hias commented 6 months ago

MSVC has an open bug that prevents a.o. importing both fmt and std modules in some use cases (i.e. depends which functionality one uses from the std) due to the mixing of std includes inside fmt itself and using import std.

For example:

error C2572: 'std::enable_if': redefinition of default argument: parameter 1

It is possible to work around this by replacing std includes inside fmt with import std instead, and including the C headers for the few types and functions defined outside the std namespace (because these are not exported from the std module):

#include <limits.h>
#include <stdint.h>
#include <stdio.h>
#include <time.h>

Maybe something worth considering with an optional feature macro?

matt77hias commented 6 months ago

tl;dr

I created a new fork with all changes (in reverse chronological order) required to use (i.e. not just build a static lib) MSVC + C++ modules + fmt:

~New MSVC bug~

phprus commented 6 months ago

Resolved warning C4127: conditional expression is constant Independent of C++ modules. Trivial to bring back to fmt.

Use const_check, like this:

https://github.com/fmtlib/fmt/blob/4e8640ed90ac8751d4a8ca500b893cc8c4bb9668/include/fmt/base.h#L2210C7-L2210C18

matt77hias commented 6 months ago

Note that defined(__cpp_if_constexpr) is used throughout the code as well, but not for just guarding the constexpr part of if constexpr (which looks like hell ;-)), though.

vitaut commented 6 months ago

Resolved warning C4127: conditional expression is constant

A PR for the conditional expression is constant warning is welcome (using const_check).

Resolved FMT_USE_CONSTEVAL for MSVC

What is the problem that you are trying to solve with this change?

Maybe something worth considering with an optional feature macro?

I would wait until the std module support is more stable.

matt77hias commented 6 months ago

A PR for the conditional expression is constant warning is welcome (using const_check).

Done

What is the problem that you are trying to solve with this change?

This is not something that is apparent when fmt is built as a static library and be done with it. Upon using this version my own wrappers (in particular the consteval constructor of fmt::basic_format_string< C, ArgTs... >) started to break because consteval support for MSVC is disabled. Note that both consteval and std::is_constant_evaluated work just fine for MSVC. Maybe the former broke at some point, but the latter has always worked for me. FWIIW my FMT_MSC_VERSION is currently 1939.

vitaut commented 6 months ago

AFAICS FMT_USE_CONSTEVAL is set correctly on MSVC: https://www.godbolt.org/z/s8WT53PY9.

matt77hias commented 6 months ago

My mistake. import std does not import the feature macros. There is a missing <version> include that should have been part of the mixing of std imports and includes bucket (i.e. 3da01e3).

matt77hias commented 6 months ago

I would wait until the std module support is more stable.

Imho, given the potential reduced build time import std (C++23) could potentially have over std includes (of which some are included more than once), it could be useful if fmt provided a feature macro to prefer one over the other. I don't see this as a temporary hack but something that has the potential of becoming the default path in the long run.

Though, the below specific snippet is a hack for sure and not something I suggest using ;-)

#define static
#include <time.h>
#undef static
ShifftC commented 6 months ago

We maintain an internal fork of fmt that utilizes std modules with a feature macro.

The error "'vformat': is not a member of 'fmt'" is due to the mixed usage of FMT_BEGIN_EXPORT and namespaces in format.h, which gets closed early by } // namespace detail. In our fork we replaced all instances of FMT_BEGIN_EXPORT and FMT_BEGIN_END with FMT_EXPORT to prevent issues like this and force to export only whats necessary.

I also can't reproduce the error with time.h, simply including the header (when using std module) works. The error usually appears when importing time.h as a header-unit as there is a bug where static inline functions can't be called.

matt77hias commented 6 months ago

Good catch. This fixes both

Error   C2039   'vformat': is not a member of 'fmt' fmt C:\Users\...\fmt\fmt\fmt\format.h   4399
Error   C2661   'fmt::v10::vformat': no overloaded function takes 2 arguments   fmt C:\Users\...\fmt\fmt\fmt\format-inl.h   153

I also can't reproduce the error with time.h, simply including the header (when using std module) works. The error usually appears when importing time.h as a header-unit as there is a bug where static inline functions can't be called.

Fwiiw in my case, the error is logged upon building module implementation units that use std::chrono::system_clock::time_point in a formatted string (e.g., {:%H:%M:%S}).

vitaut commented 6 months ago

given the potential reduced build time import std (C++23) could potentially have over std includes (of which some are included more than once), it could be useful if fmt provided a feature macro to prefer one over the other.

Fair enough. Could you submit a PR to do this (possibly in a subset of {fmt} for starters)?

matt77hias commented 5 months ago

Could you submit a PR to do this?

https://github.com/fmtlib/fmt/pull/3928

vitaut commented 5 months ago

With #3928 merged, I guess this can be closed now. The fileno warning is unrelated to modules but a PR to fix it would be welcome.

matt77hias commented 5 months ago

a PR to fix it would be welcome.

https://github.com/fmtlib/fmt/pull/3930

matt77hias commented 5 months ago

I now also modularized some Windows specific code requiring wchar_t formatting, and narrowed it down to the below:

import fmt;

auto main()
    -> int
{
    auto str = fmt::format(L"{}", 1u);

    return 0;
}
1>C:\Users\...\fmt\format.h(3565,1): fatal  error C1001: Internal compiler error.
1>(compiler file 'msc1.cpp', line 1587)
1> To work around this problem, try simplifying or changing the program near the locations listed above.
1>If possible please provide a repro here: https://developercommunity.visualstudio.com
1>Please choose the Technical Support command on the Visual C++
1> Help menu, or open the Technical Support help file for more information
1>INTERNAL COMPILER ERROR in 'C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.39.33519\bin\HostX64\x64\CL.exe'
1>    Please choose the Technical Support command on the Visual C++
1>    Help menu, or open the Technical Support help file for more information

@ShifftC do you happen to have a specific workaround (or do you only use char formatting)? 🤞

My experience so far with all the internal compiler errors for C++ modules that I encountered, is that there are workarounds. Unfortunately, it requires stripping every use case down to a few LOCs in order to isolate the actual problem.

ShifftC commented 5 months ago

If have a fork with some fixes for msvc and clang, including an ice, still trying to get it to work with gcc but that might just be my setup. Not sure if it's the same ice, currently not home so I can't test it.

matt77hias commented 5 months ago

Tried it for the above example. Unfortunately, still the same ice.

Created a ticket.

ShifftC commented 5 months ago

Compiling the example before https://github.com/fmtlib/fmt/pull/3928 gives 'fmt::v10::detail::convert_float': no matching overloaded function found instead of an ice for me. Our internal fork just fails to compile without an error 🙃. We convert everything to char before internal usage.

I have also done some testing because of the error with time.h, turns out including time.h in the global module fragment of the module using chrono formatting also 'fixes' it, at least for std::chrono::time_point and std::tm, formatting std::chrono::duration also gives an ice.

matt77hias commented 5 months ago

Upon some further trial and error, it turns out that