Custom formatter: force HASH_FLAG behavior

[polyvertex]

Hi, I was toying around with my shiny custom arg formatter and wondered what's the lightest way (speed) to modify the default behavior when HASH_FLAG is specified for integers (any input type, any output base). I would like to force the output prefix, only the prefix, to be in lower case (call me picky). So for example, the hex prefix would always be 0x even if the the X modifier is used, same goes for 0b...

Here are the options I've explored so far:

• The easiest way seems to just patch the original void BasicWriter<Char>::write_int(T value, Spec spec), but I do not wish to maintain my own fork of fmt
• I appreciate I cannot just do a spec().type_ = 'x' in the visit_any_int implementation below
• I considered doing something like writer_ << "0x" (or whatever the right syntax/name is) then remove the HASH_FLAG bit from spec().flags_ before eventually proceeding to the default visit_any_int implementation. It seems to be the most acceptable solution but I thought I would ask you first as there might be an even cleaner solution that fits better to fmt's design?

Here is the current implementation of the formatter as a starting base:

template <class CharT>
class CustomArgFormatter : public fmt::BasicArgFormatter<CustomArgFormatter<CharT>, CharT>
{
public:
    typedef fmt::BasicArgFormatter<CustomArgFormatter<CharT>, CharT> SuperT;

    CustomArgFormatter<CharT>(
            fmt::BasicFormatter<CharT, CustomArgFormatter>& f,
            fmt::FormatSpec& s, const CharT* fmt)
        : SuperT(f, s, fmt) { }

    template <typename T>
    void visit_any_int(T value)
    {
        if (!std::is_unsigned<T>::value &&
                (this->spec().type() == 'x' || this->spec().type() == 'X'))
        {
            // cast any signed integral into its unsigned equivalent
            typedef std::make_unsigned<T>::type UnsignedT;
            SuperT::visit_any_int<UnsignedT>(static_cast<UnsignedT>(value));
        }
        else
        {
            SuperT::visit_any_int<T>(value);
        }
    }
};

[vitaut]

Writing "0x" won't play well with sign and alignment. For example, fmt::print("{:+#x}", 42) should print "+0x2a", not "0x+2a". To do what you want a small change to the fmt library might be needed, namely replacing

      prefix[prefix_size++] = spec.type();

in write_int with something like

      prefix[prefix_size++] = spec.type_prefix();

with the default implementation of type_prefix() returning spec.type() as before.

Then you could provide your own version of FormatSpec with type_prefix returning lowercase x.

[polyvertex]

Ah yes! I forgot to mention I was OK with the sign issue since it would never occur in my case...
Well, if you would accept such a PR, I'm on my way then!

[vitaut]

Well, if you would accept such a PR, I'm on my way then!

Sure.

[polyvertex]

Err... Are you sure?!

Since BasicFormatter::format() instantiates FormatSpec, I thought I would add the FormatSpec type as a template parameter (i.e. template <typename Char, typename ArgFormatter, typename FormatSpecT> BasicFormatter<...>::format(...)) and then propagate that change to every "connected" classes, may it be by hierarchy or by call... But since I'm not familiar with the guts of fmt, I didn't realize it would imply so many modifications (BasicFormatter, ArgFormatter, BasicArgFormatter, internal::ArgFormatterBase, ...), I mean, I feel quite guilty of doing such a mess for such an insignificant feature now.

So unless you see a broader use of having the FormatSpec type as template parameter (I fail to see any), or you just reply "go nuts dude" (3 words, lower cased), I may find reasonable to pass on that intrusive approach...

Half-thought alternatives that do not feel right:

• Make the new FormatSpec::type_prefix() method virtual
• Add a char FormatSpec::type_prefix member

Did I miss something?

[vitaut]

I don't think you need to pass FormatSpec through the whole call hierarchy. Just add non-virtual char FormatSpec::type_prefix() and change your custom arg formatter to

template <class CharT>
class CustomArgFormatter {
...
    template <typename T>
    void visit_any_int(T value) {
         CustomFormatSpec spec(spec_);
         writer_.write_int(value, spec);
    }
};

where CustomFormatSpec is a subclass of FormatSpec that returns lowercase prefix in type_prefix.

Does it make sense?

[polyvertex]

The FormatSpec template argument thing came of the wish to avoid the creation of a new-but-same object at "visit" level. I thought it would be cleaner that way. It certainly doesn't look like it now. But my understanding is that it all comes down to exposing the features of FormatSpec to the user-side whereas for now it is a treasure well guarded by internal::ArgFormatterBase.

What I mean is that, for example, I noticed it doesn't look like it's actually possible to call writer().write_int(value, spec) from the custom formatter since write_int is private. I saw you bypassed that by declaring internal::ArgFormatterBase as a friend of BasicWriter. You certainly made the "write_int & co" methods private for a reason so even as a lumberjack-coder, I wouldn't dare to change that.

Well all of this means I need more time than I thought to make this PR (just for the sake of doing a tiny lower case conversion; fun)! I'll report here when I got stroke by the thunder with some brilliant idea instead of making noise here. Enjoy your last Christmas before Trump-era.

[vitaut]

Sorry, I forgot that write_int is private. I think the easiest solution (without exposing write_int) is to change:

template <typename T, typename Spec, typename FillChar>
  BasicWriter &operator<<(IntFormatSpec<T, Spec, FillChar> spec) {
    internal::CharTraits<Char>::convert(FillChar());
    write_int(spec.value(), spec);
    return *this;
  }

to something like

  template <typename Spec>
  typename internal::EnableIf<Spec::INTEGER, BasicWriter&>::type
      operator<<(Spec spec) {
    internal::CharTraits<Char>::convert(spec.fill());
    write_int(spec.value(), spec);
    return *this;
  }

and add

enum {INTEGER = 1};

to IntFormatSpec (would be better to check if spec.value() returns an integral type, but it is a bit complicated in C++98).

Then you'll be able to use a custom format spec:

template <typename T>
class CustomSpec : public fmt::FormatSpec {
 private:
  T value_;

 public:
  enum {INTEGER = 1};

  ...

  T value() const { return value_; }
  char fill() const { return fill_; }
};

fmt::MemoryWriter w;
w << CustomSpec<int>(42);

The good thing about this solution is that it will allow customizing formatting when using the write API too.

[polyvertex]

Nice! It works!

Still it felt a bit ugly (don't mind me) so, in the meantime and in case you care, I've modified fmt so the user can pass its own type of Spec as a template argument through the whole ArgFormatter chain. It's not that messy actually. If you feel OK to add such a feature to your lib, I would be glad to submit a PR for reviewing. I tried to follow the initial design and intend of each part, as well as the (google) code style. Be aware I'm still not fully familiar with the whole machinery though.

Usage example:

struct CustomFormatSpec : public fmt::FormatSpec
{
    char type_prefix() const { return type() | 0x20; } // blindly force lower case
};

template <class CharT, class SpecT = CustomFormatSpec>
class CustomArgFormatter : public fmt::BasicArgFormatter<
    CustomArgFormatter<CharT>, CharT, SpecT>
{
public:
    typedef
        fmt::BasicArgFormatter<CustomArgFormatter<CharT>, CharT, SpecT>
        SuperT;

    CustomArgFormatter<CharT, SpecT>(
            fmt::BasicFormatter<CharT, CustomArgFormatter>& f,
            typename SpecT& s, const CharT* fmt)
        : SuperT(f, s, fmt) { }

    template <typename T>
    void visit_any_int(T value)
    {
        const char type_lc = this->spec().type() | 0x20; // lower case
        if (!std::is_unsigned<T>::value && (type_lc == 'x' || type_lc == 'b'))
        {
            typedef std::make_unsigned<T>::type UnsignedT;
            SuperT::visit_any_int<UnsignedT>(static_cast<UnsignedT>(value));
        }
        else
        {
            SuperT::visit_any_int<T>(value);
        }
    }
};

[vitaut]

Please do submit a PR and I'll have a look.

[polyvertex]

PR #444