Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use overridden locale in ostream #1406

Merged
merged 1 commit into from
Nov 17, 2019
Merged

Conversation

dlaugt
Copy link
Contributor

@dlaugt dlaugt commented Nov 13, 2019

I'm wondering if the overridden locale can be passed to ostream. Here is a code with the current behavior:

struct S
{
  string a;
  int b;
};

ostream& operator<<(ostream& os, const S& s)
{ return os << "{" << s.a << ": " << s.b << "}"; }

locale::global (locale ("fr-FR"));
format (locale("en-US"), "{} {:n} {}", 1234, 1234, S {"test", 1234});
// gives 1234 1,234 {test: 1 234}

For known types (including chrono), format is using C locale or the overridden locale. In this example, I don't see the interest to display the result of ostream with the global locale...

I can override the current behavior by implementing the following for each of my own classes:

namespace fmt
{
  template <>
  struct formatter<S> : formatter<basic_string_view<char>, char>
  {
    using base_type = formatter<basic_string_view<char>, char>;

    auto format (const S& value, format_context& ctx)
    {
      basic_memory_buffer<char> buffer;
      internal::formatbuf<char> format_buffer (buffer);
      std::basic_ostream<char> out (&format_buffer);
      out.imbue (ctx.locale ().get<std::locale> ());
      out.exceptions (std::ios_base::failbit | std::ios_base::badbit);
      out << value;

      buffer.resize (buffer.size ());
      basic_string_view<char> str (buffer.data (), buffer.size ());
      return base_type::format (str, ctx);
    }
  };
}

But I'm wondering if this can be done as standard for ostream.

Copy link
Contributor

@vitaut vitaut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. Mostly looks good but let's get rid of the duplicate format_value overload.

Comment on lines 187 to 194
template <typename Char, typename T>
void format_value(buffer<Char>& buf, const T& value) {
formatbuf<Char> format_buf(buf);
std::basic_ostream<Char> output(&format_buf);
output.exceptions(std::ios_base::failbit | std::ios_base::badbit);
output << value;
buf.resize(buf.size());
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this and make loc default to no locale in the format_value defined in ostream.h.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've committed the change for having only one format_value. In my benchmark under msvc 2019, constructing a copy of the global locale has a 9% impact on performance results. I've added an operator bool in locale_ref to avoid this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call.

@dlaugt dlaugt force-pushed the locale-ostream branch 3 times, most recently from 5703f26 to 4162d5b Compare November 17, 2019 13:00
@vitaut vitaut merged commit c58b7d9 into fmtlib:master Nov 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants