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

fix: make std::bitset formattable again #3660

Merged
merged 2 commits into from
Oct 3, 2023
Merged

Conversation

muggenhor
Copy link
Contributor

@muggenhor muggenhor commented Sep 29, 2023

It used to be formattable via operator<<(ostream&) implicitly. Make it
formattable again, but this time via formatter specialization.

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. I think it's reasonable to provide a formatter for bitset but it shouldn't rely on ostream.

@muggenhor
Copy link
Contributor Author

muggenhor commented Sep 29, 2023

Sure, I can do that. The formatting itself is easy enough if sticking to the same as operator<<(ostream&, const bitset&) (for-loop from MSB to LSB).

But do you have any preference for a formatter to derive from or what to base a parse() on?

Edit: this last version works and without ostream. But supports no format specs whatsoever.

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.

But do you have any preference for a formatter to derive from or what to base a parse() on?

I think we should at least support width/precision/alignment. This can be done with nested_formatter, something like:

template <size_t N>
struct fmt::formatter<std::bitset<N>> : nested_formatter<string_view> {
  template <typename FormatContext>
  auto format(const std::bitset<N>& bs, FormatContext& ctx) const {
    return write_padded(ctx, [=](auto out) {
      for (auto pos = N; pos; --pos) {
        out = fmt::detail::write<char>(out, bs[pos - 1] ? '1' : '0');
      }
      return out;
    });
  }
};

but adapted to work with arbitrary code unit (character types).

https://godbolt.org/z/f6b7jhc89

include/fmt/std.h Outdated Show resolved Hide resolved
@muggenhor
Copy link
Contributor Author

I think we should at least support width/precision/alignment. This can be done with nested_formatter, something like:

Done. I also went ahead and adapted the test case to that by padding on the left with extra zeroes.

@muggenhor muggenhor force-pushed the fix/bitset branch 2 times, most recently from 30619ec to cc4409c Compare September 30, 2023 18:55
@muggenhor
Copy link
Contributor Author

Apparently nested_formatter wasn't trivially default constructible nor is it usable with lambdas on C++11 (which CI is apparently configured to build with). So had to fix the former and to resort to an old-fashioned functor for the latter.

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.

Mostly looks good, just a few more comments inline.

include/fmt/format.h Outdated Show resolved Hide resolved
include/fmt/format.h Outdated Show resolved Hide resolved
include/fmt/std.h Outdated Show resolved Hide resolved
It used to be formattable via operator<<(ostream&) implicitly. Make it
formattable again, but this time via formatter specialization.
@muggenhor
Copy link
Contributor Author

Feel free to merge this if it looks good to you

@Roman-Koshelev
Copy link
Contributor

std::bitset::to_string allows you to specify the zero and one characters. It would be great if the formatter supported this

@vitaut vitaut merged commit f76603f into fmtlib:master Oct 3, 2023
40 checks passed
@vitaut
Copy link
Contributor

vitaut commented Oct 3, 2023

Thank you!

@muggenhor muggenhor deleted the fix/bitset branch October 3, 2023 19:55
@muggenhor
Copy link
Contributor Author

Thank you for your time and feedback!

std::bitset::to_string allows you to specify the zero and one characters. It would be great if the formatter supported this

@Roman-Koshelev what syntax would you suggest for this in the format spec? Not having a clue about what syntax is a good idea here is why I didn't even attempt that.

@Roman-Koshelev
Copy link
Contributor

I believe the correct syntax should look like this
[fill[align]]["any_separator" zero[one]]["L"]

@muggenhor
Copy link
Contributor Author

Why make zero mandatory but one optional?

I understand this is practically the case for bitset::to_string too. But it isn't clear to me that that isn't more of a side effect of the way default arguments work than being an intentional design choice.

@Roman-Koshelev
Copy link
Contributor

An alternative can be
[fill[align]]["z" zero]["o" one]["L"]

@Roman-Koshelev
Copy link
Contributor

@muggenhor what do you think?

@muggenhor
Copy link
Contributor Author

@Roman-Koshelev sorry for the delayed reply. I'm super strapped for time lately.

Anyway, that syntax looks good to me. Having the mandatory z/o characters there should even make parsing somewhat easier. A colleague mentioned that maybe s(et)/c(lear) would be preferred by some people? I don't care much either way

ckerr pushed a commit to transmission/fmt that referenced this pull request Nov 7, 2023
* fix: make std::bitset formattable again

It used to be formattable via operator<<(ostream&) implicitly. Make it
formattable again, but this time via formatter specialization.

* fix: make nested_formatter constexpr default constructible
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.

None yet

3 participants