fix format a long value with formatter

[fsb4000]

Fixes #2765

Although @cpplearner wrote everything in the issue, I was able to write

typename _Format_arg_traits<_FormatContext>::template _Storage_type<_Ty>

correctly only the third time :)

[cpplearner]

P0355R7_calendars_and_time_zones_formatting is failing with:

D:\build\out\inc\format(3148) : Assertion failed: The custom handler should be structurally unreachable for _Arg_formatter

This seems to be because std::string gets stored as a std::basic_format_arg::handle, which means this function is unused:

STL/stl/inc/format

Lines 1778 to 1780 in 17fde2c

	template <class _Traits, class _Alloc>
	static auto _Phony_basic_format_arg_constructor(const basic_string<_Char_type, _Traits, _Alloc>&)
	-> basic_string_view<_Char_type>; // not defined

Maybe the constrained overload is considered to be more specialized?

STL/stl/inc/format

Lines 1744 to 1745 in 17fde2c

	template <_Has_formatter<_Context> _Ty>
	static auto _Phony_basic_format_arg_constructor(_Ty&&) {

[StephanTLavavej]

Thanks for fixing this bug! I pushed minor stylistic changes and a small test coverage increase - please meow if you disagree.

[StephanTLavavej]

Thanks @cpplearner for noticing that P0645R10_text_formatting_custom_formatting already has machinery that's identical to Box<T>:

STL/tests/std/tests/P0645R10_text_formatting_custom_formatting/test.cpp

Lines 86 to 97 in 17fde2c

	template <class T>
	struct custom_formattable_type {
	T val;
	};
	template <class T, class charT>
	struct std::formatter<custom_formattable_type<T>, charT> : std::formatter<T, charT> {
	template <class FC>
	auto format(const custom_formattable_type<T>& val, FC& format_ctx) const {
	return std::formatter<T, charT>::format(val.val, format_ctx);
	}
	};

I verified that adding test coverage for long here will fail without @fsb4000's fix. To be comprehensive, I also added coverage for unsigned long and long double.

[fsb4000]

This seems to be because std::string gets stored as a std::basic_format_arg::handle, which means this function is unused:

No, std::string is ok, I have debugged the test.
The error is from the asserts:

STL/tests/std/tests/P0355R7_calendars_and_time_zones_formatting/test.cpp

Line 1094 in 17fde2c

test_information_classes<wchar_t>();

test_information_classes<char>(); works OK. (strange)

and inside test_information_classes

STL/tests/std/tests/P0355R7_calendars_and_time_zones_formatting/test.cpp

Lines 817 to 826 in 17fde2c

	const local_info nonexistent2 = la_tz->get_info(LA::Std_1.local_end());
	// N4885 [time.zone.info.sys]/7: "Effects: Streams out the sys_info object r in an unspecified format."
	// N4885 [time.zone.info.local]/3: "Effects: Streams out the local_info object r in an unspecified format."
	empty_braces_helper(sys1,
	STR("begin: 2020-04-04 16:00:00, end: 2020-10-03 16:00:00, "
	"offset: 36000s, save: 0min, abbrev: AEST"),
	STR("begin: 2020-04-04 16:00:00, end: 2020-10-03 16:00:00, "
	"offset: 36000s, save: 0min, abbrev: GMT+10"));

All empty_braces_helper tests with local_info and sys_info are failed

STL/tests/std/tests/P0355R7_calendars_and_time_zones_formatting/test.cpp

Lines 234 to 239 in 17fde2c

	void empty_braces_helper(
	const Arg& val, const CharT* const expected, const same_as<const CharT*> auto... alternatives) {
	// N4885 [time.format]/6: "If the chrono-specs is omitted, the chrono object is formatted
	// as if by streaming it to std::ostringstream os and copying os.str() through the output iterator
	// of the context with additional padding and adjustments as specified by the format specifiers."
	const auto result = format(STR("{}"), val);

So format local_info is stored as a std::basic_format_arg::handle

[cpplearner]

• format(STR("{}"), val) calls operator<<;
• operator<< calls _Widen_string<_CharT>(_Val.abbrev);
• _Widen_string returns a const string& when CharT is char, but returns a wstring when CharT is wchar_t.

That const probably makes a difference here.

#include <format>
#include <string>

using namespace std;

using A = _Format_arg_traits<format_context>::_Storage_type<string>;
using B = _Format_arg_traits<format_context>::_Storage_type<const string>;

static_assert(is_same_v<A, basic_format_arg<format_context>::handle>);
static_assert(is_same_v<B, string_view>);

[fsb4000]

well done @cpplearner !
So can we change

template <class _Traits, class _Alloc> 
 static auto _Phony_basic_format_arg_constructor(const basic_string<_Char_type, _Traits, _Alloc>&) 
     -> basic_string_view<_Char_type>; // not defined 

to

template <class _Traits, class _Alloc> 
 static auto _Phony_basic_format_arg_constructor(basic_string<_Char_type, _Traits, _Alloc>) 
     -> basic_string_view<_Char_type>; // not defined 

?

[fsb4000]

Yes, I did it intentionally because we don't modify basic_format_arg.
I would store char* as const char* if I don't plan to modify it.
But now I am unsure, maybe the const is not needed...

[barcharcraz]

Approved, despite this not doing what the standard says for basic_string&

Agree with Casey that we need to reformulate how this overload set works, it's been a very consistent source of bugs in both our implementation and the standard.

[StephanTLavavej]

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

[StephanTLavavej]

Thanks for the bugfix and targeted overhaul here, improving our implementation beyond the Standard itself! 😹 🐞 🎉