Runtime exception instead of compile time error when formatting a user type.

[ScottLangham]

I tried to format a CAtlString (from header atlstr).
I'm using Visual Studio 2013 update 5.

With the following code, I expected that if cppformat isn't aware of CAtlString, I'd get a compile-time error and have to provide an override or template specialization to explain to cppformat what to do. But, I get a run time exception instead.

        ATL::CAtlString s("hi");
        auto formattedString = fmt::sprintf("Hello %s", s);
        auto result = std::string("Hello hi");
        Assert::AreEqual(result, formattedString);

Am I doing something wrong? This feels a bit error prone as it would be easy to code this by mistake.

[vitaut]

Does CAtlString provide conversion operators? Also what is the error message?

[ScottLangham]

I depends what kind of conversion operator you mean. I think it provides a conversion to const char*.

The throw is on line 436 of format.cc (the first throw in this func);

FMT_FUNC void fmt::internal::report_unknown_type(char code, const char *type) {
  if (std::isprint(static_cast<unsigned char>(code))) {
    FMT_THROW(fmt::FormatError(
        fmt::format("unknown format code '{}' for {}", code, type)));
  }
  FMT_THROW(fmt::FormatError(
      fmt::format("unknown format code '\\x{:02x}' for {}",
        static_cast<unsigned>(code), type)));
}

This is the call stack I get:

MyTests.dll!fmt::internal::report_unknown_type(char code, const char * type) Line 436   C++
MyTests.dll!fmt::internal::PrintfFormatter<char>::format(fmt::BasicWriter<char> & writer, fmt::BasicStringRef<char> format_str, const fmt::ArgList & args) Line 904 C++
MyTests.dll!fmt::printf<char>(fmt::BasicWriter<char> & w, fmt::BasicStringRef<char> format, fmt::ArgList args) Line 2375    C++
MyTestsTests.dll!fmt::sprintf(fmt::BasicStringRef<char> format, fmt::ArgList args) Line 2388    C++
MyTestsTests.dll!fmt::sprintf<ATL::CStringT<wchar_t,ATL::StrTraitATL<wchar_t,ATL::ChTraitsCRT<wchar_t> > > >(fmt::BasicStringRef<char> arg0, const ATL::CStringT<wchar_t,ATL::StrTraitATL<wchar_t,ATL::ChTraitsCRT<wchar_t> > > & <args_0>) Line 2631   C++
MyTestsTests.dll!AsWin32_UnitTests::FormatStringTests::FormatString_CAtlString() Line 134   C++
MyTestsTests.dll!Microsoft::VisualStudio::CppUnitTestFramework::TestClass<AsWin32_UnitTests::FormatStringTests>::__Invoke(void (void) * method) Line 465    C++

[ScottLangham]

The value of 'code' is: 115 's'
The value of type is: "object"

[vitaut]

fmt::(s)printf gives a compile-time error if the argument cannot be formatted (not a recognized built-in type and no suitable ostream operator<< overload). For example,

#include "format.h"

struct S {};

int main() {
  fmt::printf("%s", S());
}

gives

test.cc:6:24:   required from here
format.h:2565:6: error: no match for ‘operator<<’ (operand types are ‘std::basic_ostringstream<char>’ and ‘const S’)
   os << value;
      ^

So I guess CAtlString provides operator<< and the problem here is that sprintf doesn't have a format specifier for objects. fmt::sprintf is there mostly for migrating old code that uses standard sprintf and shouldn't be passing objects to sprintf in the first place. For newer code I recommend using fmt::format and friends. Does the following work for you?

ATL::CAtlString s("hi");
auto formattedString = fmt::format("Hello {}", s);

[ScottLangham]

formattedString ends up with: "Hello 059E8800"
I'll see if I can debug and spot how it ends up with that.

I am working with many old log statements that I wanted to add some type safety to, so I need to stick with the old printf style format strings.

I have tried boost::format and that seems to work ok and gives me a compiler error, but I was hoping to avoid the bloat and compile slowness that boost::format brings by using cppformat instead.

[ScottLangham]

When using fmt::format("Hello {}", s), as you suggest, the debugger ends up in the format function of format.h at line 2246:

template <typename Char, typename T>
void format(BasicFormatter<Char> &f, const Char *&format_str, const T &value) {
  std::basic_ostringstream<Char> os;
  os << value;

On the os << value line, it calls the conversion operator on the CAtlString which returns a const char* to the correct string value. But, the overload of << that gets called is the one that accepts a const void*, so it writes out the pointer to the string instead of the contents of it.

Here's the stack up to calling the conversion operator:

Axe_UnitTests.dll!ATL::CSimpleStringT<wchar_t,0>::operator wchar_t const *() Line 377   C++
Axe_UnitTests.dll!fmt::format<char,ATL::CStringT<wchar_t,ATL::StrTraitATL<wchar_t,ATL::ChTraitsCRT<wchar_t> > > >(fmt::BasicFormatter<char> & f, const char * & format_str, const ATL::CStringT<wchar_t,ATL::StrTraitATL<wchar_t,ATL::ChTraitsCRT<wchar_t> > > & value) Line 2246   C++
    Axe_UnitTests.dll!fmt::internal::MakeValue<char>::format_custom_arg<ATL::CStringT<wchar_t,ATL::StrTraitATL<wchar_t,ATL::ChTraitsCRT<wchar_t> > > >(void * formatter, const void * arg, void * format_str_ptr) Line 809  C++
    Axe_UnitTests.dll!fmt::internal::ArgFormatter<char>::visit_custom(fmt::internal::Value::CustomValue c) Line 610 C++
    Axe_UnitTests.dll!fmt::internal::ArgVisitor<fmt::internal::ArgFormatter<char>,void>::visit(const fmt::internal::Arg & arg) Line 1000    C++
    Axe_UnitTests.dll!fmt::BasicFormatter<char>::format(const char * & format_str, const fmt::internal::Arg & arg) Line 1052    C++
    Axe_UnitTests.dll!fmt::BasicFormatter<char>::format(fmt::BasicStringRef<char> format_str, const fmt::ArgList & args) Line 1072  C++
    Axe_UnitTests.dll!fmt::BasicWriter<char>::write(fmt::BasicStringRef<char> format, fmt::ArgList args) Line 1708  C++
    Axe_UnitTests.dll!fmt::format(fmt::BasicStringRef<char> format_str, fmt::ArgList args) Line 2330    C++
    Axe_UnitTests.dll!fmt::format<ATL::CStringT<wchar_t,ATL::StrTraitATL<wchar_t,ATL::ChTraitsCRT<wchar_t> > > >(fmt::BasicStringRef<char> arg0, const ATL::CStringT<wchar_t,ATL::StrTraitATL<wchar_t,ATL::ChTraitsCRT<wchar_t> > > & <args_0>) Line 2625   C++
    Axe_UnitTests.dll!AsWin32_UnitTests::FormatStringTests::FormatString_CAtlString() Line 134  C++

[vitaut]

I see that CStringT is parameterized on wchar_t. If you want to format a wide string, you should use a wide format string:

fmt::format(L"Hello {}", s)

Printing a pointer instead of a string when mixing wide and narrow strings is a known issue with IOStreams:

#include <iostream>
int main() {
  std::cout << L"test"; // prints a pointer, e.g. 0x400844
}

I'll look if I can add a compile-time error when formatting objects with fmt::*printf.

[vitaut]

@ScottLangham Thinking more of it, I came to a conclusion that there is no reason for fmt::(s)printf to give an error at all in this case. Instead it should format an object as fmt::print does. So the following should be allowed now:

ATL::CAtlString s("hi");
auto formattedString = fmt::sprintf(L"Hello %s", s); // result is "Hello hi"

although I haven't tested this with CAtlString. Hope this works for you.

[vitaut]

Here's the commit that implements this: 79d8f59

[vitaut]

Closing for now, but feel free to reopen if there are any issues with this.

[ScottLangham]

Hi Victor, thanks for your help so far. I've been unable to get to this for a few days.

I've tried your latest and it formats ok using char based strings, but I'm using Windows and need wide strings to work. So, I added this after including format.h:

namespace fmt
{
    inline std::wstring sprintf(WCStringRef format, ArgList args)
    {
        WMemoryWriter w;
        fmt::printf(w, format, args);
        return w.str();
    }
    FMT_VARIADIC_W(std::wstring, sprintf, WCStringRef);
}

Then, I try this unit test:

        ATL::CAtlStringW s(L"hi");
        auto formattedString = fmt::sprintf(L"Hello %s", s);
        auto result = std::wstring(L"Hello hi");
        Assert::AreEqual(result, formattedString);

But, I still get a pointer value instead of hi in the formatted string. It does work though if I use an ATL::CAtlStringA and single byte strings.

Looking at the commit you made, I notice that you have: const char *format = "}";. As that hardcodes to char it makes me think that cppformat doesn't fundamentally provide support for wide strings. But, in places the code does seem to templatize on string type, so I can't determine if this is intended to be supported or not. Should it work?

Thanks,
-Scott

[vitaut]

Looking at the commit you made, I notice that you have: const char *format = "}";. As that hardcodes to char it makes me think that cppformat doesn't fundamentally provide support for wide strings. But, in places the code does seem to templatize on string type, so I can't determine if this is intended to be supported or not. Should it work?

No, it shouldn't for wide strings (it wasn't triggered before because there was no wide string sprintf overload). Should be fixed in ef710de. Thanks for catching this.

I'm not sure why formatting of ATL::CAtlStringW doesn't work and I don't have Windows machine at hand to check. Without ef710de I'd expect an exception being thrown instead of a pointer value printed. I've added a test case in ef710de#diff-91c2fe56034a55ef7804f289890e6f1cR460, but couldn't reproduce the issue - perhaps there's some weird implicit conversion going on. Does it reach void format(BasicFormatter<Char> &f, const Char *&format_str, const T &value) when you call fmt::sprintf(L"Hello %s", ATL::CAtlStringW(L"hi"))?

[mwinterb]

Scott, vitaut:
The basic issue is an unfortunate interaction with the implicit conversion operator on CAtlStringW to const wchar_t*. Outside of CAtlString, this should replicate the same behavior:

#include <iostream>
struct A
{
    operator wchar_t const*() const
    {
        return L"hi";
    }
};
int main()
{
    A s;
    std::wcout << s << '\n';
    std::wcout << static_cast<const wchar_t*>(s) << '\n';
}

Although in both cases, the conversion operator is executed, the first case picks up the inserter for const void*, but the second picks up the desired overload.

I don't know if there's a decent solution for c++format, possibly providing an optional format-atl.h that does what is desired for ATL::CAtlString? One thing that Scott could possibly do is provide a basic_ostream inserter for ATL::CAtlString so that it does the correct thing.