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

Compile failure with MSVC with /W4 /WX - ostream.h(153): warning C4127: conditional expression is constant #4314

Closed
stefan301 opened this issue Jan 17, 2025 · 6 comments · Fixed by #4322

Comments

@stefan301
Copy link

The following Code doesn't compile with 'Treat warnings as errors' and warning level 4:

#include <fmt/format.h>
#include <fmt/ostream.h>
#include <iostream>

int main()
{
	fmt::print(std::cerr, "Don't {}!", "panic");
}

compiling with cl /EHsc /std:c++20 /W4 /WX /utf-8 x.cpp /MDd /I D:\libs\fmt\include D:\libs\fmt\x64\Debug\fmtd.lib gives:

D:\libs\fmt\include\fmt/ostream.h(153): error C2220: the following warning is treated as an error
D:\libs\fmt\include\fmt/ostream.h(153): warning C4127: conditional expression is constant
D:\libs\fmt\include\fmt/ostream.h(153): note: consider using 'if constexpr' statement instead

The problem is this if in print because detail::use_utf8 is a compile time constant

if (detail::use_utf8) return vprint(os, fmt.str, vargs);

Changing print this way might solve the problem for C++17 and higher but doesn't compile with C++14

FMT_EXPORT template <typename... T>
void print(std::ostream& os, format_string<T...> fmt, T&&... args) {
  fmt::vargs<T...> vargs = {{args...}};
  if constexpr( detail::use_utf8 )
  {
          return vprint( os, fmt.str, vargs );
  }
  else
  {
          auto buffer = memory_buffer();
          detail::vformat_to( buffer, fmt.str, vargs );
          detail::write_buffer( os, buffer );
  }
}
@vitaut
Copy link
Contributor

vitaut commented Jan 17, 2025

Fixing all possible warnings (especially level 4 which have tons of false positive) is a non-goal but you can suppress them with FMT_SYSTEM or other means.

@vitaut vitaut closed this as completed Jan 17, 2025
@vitaut vitaut added the wontfix label Jan 17, 2025
@ZehMatt
Copy link
Contributor

ZehMatt commented Jan 21, 2025

I also ran into this while bumping the dependency verison, wouldn't something like following solve the problem?

if FMT_CONSTEXPR (detail::use_utf8) return vprint(os, fmt.str, vargs);

This will probably not work for C++14 so it might be worth to have FMT_CONSTEXPR17 just like FMT_CONSTEXPR20

Another benefit would be that this condition is shifted to compile time and not runtime, I know optimizations are pretty clever but that will not happen for debug builds while with constexpr it will be a certain thing.

@phprus
Copy link
Contributor

phprus commented Jan 21, 2025

@ZehMatt

Use const_check function, like this:

if (detail::const_check(!detail::use_utf8))

@ZehMatt
Copy link
Contributor

ZehMatt commented Jan 21, 2025

@ZehMatt

Use const_check function, like this:

fmt/include/fmt/base.h

Line 2913 in aabe639

if (detail::const_check(!detail::use_utf8))

Does that mean I should create a PR? We consume the library directly from this repo so a local patch wouldn't be ideal.

@ZehMatt
Copy link
Contributor

ZehMatt commented Jan 21, 2025

Also it seems that detail::const_check exists to suppress the warning which isn't the same as actually having it constexpr. What is the reason behind this instead of just adding constexpr where the value is known compile time?

@vitaut
Copy link
Contributor

vitaut commented Jan 21, 2025

A PR to add const_check would be welcome. I think if constexpr is an overkill for this case and it won't handle other cases with mixed compile-time and runtime conditions without complicating the code.

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 a pull request may close this issue.

4 participants