add make_printf_args and make_wprintf_args functions #934
Conversation
add minimal test for make_printf_args and make_wprintf_args to printf-test.cc
-removed Context parameter from make_printf_args and make_wprintf_args -renamed tests
|
Hmm, not sure why the g++ 4.8/4.4 are failing. Any ideas? |
There was a problem hiding this comment.
Thanks for the PR. Mostly looks good, just small comments inline and the new tests don't compile on older gcc because implicit conversion to printf_args is not working there. You need to do something like (see the implementation of sprintf:
format_arg_store<context, Args...> as{args...};
vsprintf(..., basic_format_args<context>(as));
include/fmt/printf.h
Outdated
| typedef basic_format_args<printf_context<internal::buffer>::type> printf_args; | ||
| typedef basic_format_args<printf_context<internal::wbuffer>::type> wprintf_args; | ||
| typedef printf_context<internal::buffer>::type printf_char_context; | ||
| typedef printf_context<internal::wbuffer>::type printf_wchar_context; |
There was a problem hiding this comment.
printf_char_context and printf_wchar_context should be called printf_context and wprintf_context for consistency with format_context and wformat_context. The old printf_context template should be renamed to basic_printf_context.
There was a problem hiding this comment.
I ended up using basic_printf_context_t because it caused a name conflict with the inner typedef. That naming style is also with the format context
template <typename OutputIt, typename Char = char>
//using format_context_t = basic_format_context<OutputIt, Char>;
struct format_context_t { typedef basic_format_context<OutputIt, Char> type; };
I hope that is ok with you.
test/printf-test.cc
Outdated
| @@ -518,3 +518,19 @@ void check_format_string_regression(fmt::string_view s, const Args&... args) { | |||
| TEST(PrintfTest, CheckFormatStringRegression) { | |||
| check_format_string_regression("%c%s", 'x', ""); | |||
| } | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
nit: no need for an extra empty line
rename printf_char_context to printf_context remane printf_wchar_context to wprintf_context renamed the old printf_context template to basic_printf_context_t. the original wish was to rename it basic_printf_context, but that clashed with the name of the inner typedef. this style matches the format_context_t struct. try to fix compile errors for older gcc versions by storing a temporary.
…xample again storing the value in a temporary causes even more failures
#define out problematic test for gcc 4
|
I don't know how to make the problematic tests pass for the gcc versions and I want a tests that mirrors the intended usage, so I #defined it out for gcc 4, for lack of a better idea. |
|
In the additional test, the type returned by fmt seems to be different for msvc 2013. It returns the string literals as const array where in all of the other cases it isn't const. That kinda seems like a bug in fmtlib. |
test/printf-test.cc
Outdated
| @@ -518,3 +517,43 @@ void check_format_string_regression(fmt::string_view s, const Args&... args) { | |||
| TEST(PrintfTest, CheckFormatStringRegression) { | |||
| check_format_string_regression("%c%s", 'x', ""); | |||
| } | |||
|
|
|||
| TEST( PrintfTest, VSPrintfMakeArgsExample ) { | |||
There was a problem hiding this comment.
nit: spaces after ( and before ) are not needed
There was a problem hiding this comment.
I removed all the superfluous spaces clang formatting introduced. I think I got them all.
|
There are a few compile errors on MSVC2013: https://ci.appveyor.com/project/vitaut/fmt/builds/20446501/job/tu84x312yjrwp6h7 |
|
Merged, thanks! |
|
@tnovotny, could you please review the updated CONTIBUTING document, particularly the part about licensing, and let me know if you agree with it being applied to your contributions to {fmt}? The library is likely to be relicensed (#1073) so I'm collecting approval from all earlier contributors. Thanks! |
Add utitlity functions to allow the easy creation of
fmt::printf_argsandfmt::wprintf_args.