feat: add formatter specialization #86
Conversation
wgtmac
commented
Apr 23, 2025
wgtmac
commented
- Added std::formatter specialization for std::map, std::unordered_map and std::vector when their elements are also formattable.
- Added std::formatter specialization for classes that have a ToString at hands.
wgtmac
force-pushed
the
add_formatter
branch
2 times, most recently
from
f30ab7f to
9dfeade
Compare
src/iceberg/util/formatter.h
Outdated
|
|
||
| /// \brief std::formatter specialization for std::vector | ||
| template <typename T> | ||
| struct std::formatter<std::vector<T>> : std::formatter<std::string_view> { |
There was a problem hiding this comment.
FWIW, I think the "idiomatic" way is supposed to be std::format::join and ranges. I wonder if specializing on std types may come back to bite us later.
Example: https://stackoverflow.com/a/77993463
There was a problem hiding this comment.
Or another example, albeit C++23: https://stackoverflow.com/a/71196142
There was a problem hiding this comment.
I think we need to rename this file to formatter_internal.h so we don't expose it to the downstream. In this approach, we take full control of it.
There was a problem hiding this comment.
SGTM.
It might be good to expose formatter specializations on our own types, just so long as they're separate from the specializations for std types
There was a problem hiding this comment.
Ah ok I see that's what you've done.
test/formatter_test.cc
Outdated
| std::map<std::string, std::vector<int>> nested_map = { | ||
| {"primes", {2, 3, 5, 7, 11}}, {"fibonacci", {1, 1, 2, 3, 5, 8, 13}}}; | ||
| std::string result = std::format("{}", nested_map); | ||
| EXPECT_TRUE(result.find("primes") != std::string::npos); |
There was a problem hiding this comment.
Note that you can do EXPECT_THAT(result, testing::HasSubstr("primes")); which will also give a much better error on failure
src/iceberg/util/formatter.h
Outdated
| } | ||
|
|
||
| // Format key (handle if it's a smart pointer) | ||
| if constexpr (requires { *key; }) { |
There was a problem hiding this comment.
Won't this also fire on something like std::optional that might be undesirable?
There was a problem hiding this comment.
I've modified the concept to strictly check smart pointer type.
|
I've moved specialization for std containers to the internal header file to not pollute downstream. Let me know what you think @lidavidm |
|
LGTM |
zhjwpku
left a comment
zhjwpku
left a comment
There was a problem hiding this comment.
Impressive, I used chatgpt to help me understanding the C++20 std::views::transform, thanks for working on this.
|
Thanks @wgtmac for working on this, and thanks @lidavidm, @zhjwpku and @yingcai-cy for the review 🙌 |
5 participants
