Restructure printf_arg_formatter to make it customizable #1093
Conversation
include/fmt/printf.h
Outdated
| // handler. | ||
| template<typename OutputIt, typename Context, typename FormatSpecs, | ||
| typename ArgFormatter> | ||
| class printf_arg_formatter_proxy : public internal::function< |
There was a problem hiding this comment.
A proxy shouldn't be needed. ArgFormatter should be able to deal with both T and handle case.
There was a problem hiding this comment.
I agree that this is not really a pretty and clean way. But the problem I tried to solve is, that the printf_arg_formatter did construct a basic_printf_context containing a reference to printf_arg_formatter again. When I try to extend the printf_arg_formatter the basic_printf_context would still be constructed with printf_arg_formatter as template parameter and ignore my child class (and its overloaded operator()).
I can also not pass my formatter child class via a template parameter to printf_arg_formatter since I would disable the possibility to specify a default parameter for the forward declaration of printf_arg_formatter (on line 198).
I'm open to implement a complete different approach.
There was a problem hiding this comment.
basic_printf_context should not be parameterized on ArgFormatter, it's just an artefact of some earlier design. Please remove it and then you'll be able to address other comments on the PR.
There was a problem hiding this comment.
When I understand you correctly you mean the ArgFormatter template parameter from basic_printf_context.
That one is still used for the visit_format_arg call in basic_printf_context::format at the end. This was the problem I stumbled over before I decided to make the proxy class because I couldn't come up with another alternative. But I will stick my head into it maybe I've overlooked something or I find a cleaner way.
Would it be better to pass the ArgFormatter as template parameter for the format function directly?
There was a problem hiding this comment.
When I understand you correctly you mean the ArgFormatter template parameter from basic_printf_context.
Yes.
Would it be better to pass the ArgFormatter as template parameter for the format function directly?
Sure.
| @@ -212,14 +212,12 @@ class printf_arg_formatter | |||
| : public internal::function< | |||
| typename internal::arg_formatter_base<Range>::iterator>, | |||
| public internal::arg_formatter_base<Range> { | |||
| private: | |||
| public: | |||
There was a problem hiding this comment.
I don't think these should be public because the children won't be able to use these types directly anyway.
There was a problem hiding this comment.
Agreed. But I have to keep the iterator public (like in the arg_formatter) to get my approach working.
There was a problem hiding this comment.
Right, making iterator public makes sense.
include/fmt/printf.h
Outdated
| @@ -306,12 +304,6 @@ class printf_arg_formatter | |||
| write_null_pointer(char_type()); | |||
| return this->out(); | |||
| } | |||
|
|
|||
| /** Formats an argument of a custom (user-defined) type. */ | |||
| iterator operator()(typename basic_format_arg<context_type>::handle handle) { | |||
There was a problem hiding this comment.
This should remain here. Please see arg_formatter for an example.
There was a problem hiding this comment.
See comment at printf_arg_formatter_proxy
test/custom-printf-formatter-test.cc
Outdated
| EXPECT_EQ("1.00", custom_format("%.2f", 1.00001)); | ||
| EXPECT_EQ("-1.00", custom_format("%.2f", -1.00001)); | ||
| } | ||
| #endif |
There was a problem hiding this comment.
Please move this to printf-test.
|
Sorry for the delay. I'm still struggling with a problem (looks like a bug) of the msvc 18.x compiler. I set up a test repository and try to fix the problem there first. |
include/fmt/printf.h
Outdated
| class basic_printf_context; | ||
|
|
||
|
|
There was a problem hiding this comment.
Please remove unnecessary whitespace changes.
include/fmt/printf.h
Outdated
| @@ -246,7 +245,7 @@ class printf_arg_formatter | |||
|
|
|||
| template <typename T, FMT_ENABLE_IF(std::is_integral<T>::value)> | |||
| iterator operator()(T value) { | |||
| // MSVC2013 fails to compile separate overloads for bool and char_type so | |||
| // MSVC2013 fails to compile separate overloads for bool and char_type so | |||
| #if FMT_MSC_VER > 0 && FMT_MSC_VER <= 1804 | ||
| template <typename T, FMT_ENABLE_IF(std::is_floating_point<T>::value)> | ||
| iterator operator()(T value) { | ||
| #else |
There was a problem hiding this comment.
Can we use the above SFINAE-based definition for all compilers and remove conditional compilation (adding a comment clarifying that this is a workaround for MSVC).
There was a problem hiding this comment.
Unfortunately this doesn't work. Other compilers (clang and gcc as far as I recall) complain then.
I agree that my contributions are licensed under the {fmt} license, and agree to future changes to the licensing.
I work for the Profidata AG which is using the libfmt version 4.1.0. A former colleague of mine (spacemoose) requested the expandability of the PrintfArgFormatter (now printf_arg_formatter) in the following issue: #335.
I have the task to update the libfmt dependency to 5.3.0 and ensure that we still have this feature available. Since I have a little bit of time pressure to finish this I had to balance the time of learning the code structure and the fixing itself. So I hope my approach in this change is ok.
Brief explanation what I did:
In order to be able to override the behaviour of the printf_arg_formatter I had to decouple the printf_arg_formatter and the basic_printf_context by creating a new class printf_arg_formatter_proxy. This new class keeps the context (so we can remove it from printf_arg_formatter) and decides either to call the basic_format_arg::handle with its context instance or a argument formatter which is passed by a template parameter. To simplify the setup I overloaded vprintf which accepts an argument formatter as template argument. I wrote a test (custom-printf-formatter-test.cc) for it.
Looking forward hearing from you.