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

Anything changed around fmt::print(ostream&) ? #2936

Closed
mark-99 opened this issue Jun 11, 2022 · 6 comments
Closed

Anything changed around fmt::print(ostream&) ? #2936

mark-99 opened this issue Jun 11, 2022 · 6 comments

Comments

@mark-99
Copy link

mark-99 commented Jun 11, 2022

This code worked in 8.1.1, and works if I uncomment the lines to use ostream op<<() instead, but with trunk the fmt::print() gets an assert fail inside the CRT:

template<typename Container>
inline bool saveStatsToCSV(std::filesystem::path const& filename, std::string_view header, Container&& samples)
{
    std::ofstream of(filename);
    if (!of)
    {
        SPDLOG_ERROR("Couldn't open file '{}' for writing", filename);
        return false;
    }
    else
    {
        fmt::print(of, "{}\n", header);
        //of << header << "\n";

        for (auto const& sample : samples)
        {
            fmt::print(of, "{}\n", sample);
            //of << sample << "\n";
        }
    }

    return true;
}

Call stack:

ucrtbased.dll!_fileno(_iobuf * public_stream) Line 17
	at minkernel\crts\ucrt\src\appcrt\stdio\fileno.cpp(17)
MdpCpp.exe!fmt::v8::detail::print(_iobuf * f, fmt::v8::basic_string_view<char> text) Line 1502
	at C:\Temp\cpm-source-cache\fmt\a27d11503e3ed8119b8700c55ac8a5449cdb5537\include\fmt\format-inl.h(1502)
MdpCpp.exe!fmt::v8::detail::write(std::basic_filebuf<char,std::char_traits<char>> & buf, fmt::v8::basic_string_view<char> data) Line 86
	at C:\Temp\cpm-source-cache\fmt\a27d11503e3ed8119b8700c55ac8a5449cdb5537\include\fmt\ostream.h(86)
MdpCpp.exe!fmt::v8::detail::write_buffer<char>(std::basic_ostream<char,std::char_traits<char>> & os, fmt::v8::detail::buffer<char> & buf) Line 98
	at C:\Temp\cpm-source-cache\fmt\a27d11503e3ed8119b8700c55ac8a5449cdb5537\include\fmt\ostream.h(98)
MdpCpp.exe!fmt::v8::vprint<char>(std::basic_ostream<char,std::char_traits<char>> & os, fmt::v8::basic_string_view<char> format_str, fmt::v8::basic_format_args<fmt::v8::basic_format_context<fmt::v8::appender,char>> args) Line 158
	at C:\Temp\cpm-source-cache\fmt\a27d11503e3ed8119b8700c55ac8a5449cdb5537\include\fmt\ostream.h(158)
MdpCpp.exe!fmt::v8::print<std::basic_string_view<char,std::char_traits<char>> &>(std::basic_ostream<char,std::char_traits<char>> & os, fmt::v8::basic_format_string<char,std::basic_string_view<char,std::char_traits<char>> &> fmt, std::basic_string_view<char,std::char_traits<char>> & args) Line 174
	at C:\Temp\cpm-source-cache\fmt\a27d11503e3ed8119b8700c55ac8a5449cdb5537\include\fmt\ostream.h(174)

_fileno() asserts in format-inl.h because f==nullptr:

namespace detail {
FMT_FUNC void print(std::FILE* f, string_view text) {
#ifdef _WIN32
  auto fd = _fileno(f);

Any ideas? Clearly there's an easy workaround in this particular case, but would rather know why it's failing.

@mwinterb
Copy link
Contributor

8751a03 is the commit that changed the behavior. If you can find out why get_file is returning nullptr that'd be helpful. I may be able to look into it this weekend, but since you have a repro set up...

@mark-99
Copy link
Author

mark-99 commented Jun 11, 2022

Minimal repro:

int main()
{
    std::ofstream of("c:\\Temp\\Test.txt");
    if (!of)
        return 1;

    fmt::print(of, "{}", "Hello\n");
    return 0;
}

It gets here ok (ostream.h):

inline bool write(std::filebuf& buf, fmt::string_view data) {
  print(get_file(buf), data);
  return true;
}

Then get_file(buf) calls this:

// Formatting of built-in types and arrays is intentionally disabled because
// it's handled by standard (non-ostream) formatters.
template <typename Char> FILE* get_file(std::basic_filebuf<Char>&) {
  return nullptr;
}

so now it's print(nullptr, data) and bang.

So something is screwy. Also I'd suggest return nullptr would be better as e.g. static_assert() (or even regular assert), or similar.

@vitaut
Copy link
Contributor

vitaut commented Jun 12, 2022

It's mostly a matter of adding a null check in

print(get_file(buf), data);
and a test case. A PR is welcome.

@mark-99
Copy link
Author

mark-99 commented Jun 12, 2022

How does that fix the actual problem, ie the code does not work? I want it to write the string to the file, not fail in a different way.

@vitaut
Copy link
Contributor

vitaut commented Jun 12, 2022

It should fallback to writing to ostream (by returning false from write on null), not fail. This is already what happens in other cases when there is no file descriptor.

@vitaut
Copy link
Contributor

vitaut commented Jun 16, 2022

Fixed in eaa8efb. Thanks for reporting.

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

No branches or pull requests

3 participants