This adds an incomplete version where the specializations for the
format_kinds are disabled dummy formatters.
Implements part of
- P2585R0 Improving default container formatting
Details
Details
- Reviewers
ldionne vitaut - Group Reviewers
Restricted Project - Commits
- rGd184958bad5c: [libc++][format] Adds range-default-formatter.
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| libcxx/include/__format/concepts.h | ||
|---|---|---|
| 63–66 | We definitely don't want to use the concept from P2165 because it includes array (maybe clarify this in the comment). The naming they took is very unfortunate but at least it's exposition-only. | |
| 68–70 | This is actually not tuple-like but pair-like so I suggest renaming to __fmt_pair_like. | |
| libcxx/include/__format/range_default_formatter.h | ||
| 40–41 | What does this shadow? | |
| 58 | nit: __get_format_format looks a bit repetitive, maybe rename to __get_range_format (since it only applies to ranges) or something else? | |
| libcxx/include/__type_traits/is_specialization.h | ||
| 36–40 | This is not related to formatting and should probably be split into a separate diff. | |
Comment Actions
Thanks for the review!
| libcxx/include/__format/concepts.h | ||
|---|---|---|
| 63–66 | Correct, I added comment to avoid possible confusion between the two concepts. | |
| 68–70 | Good point! | |
| libcxx/include/__format/range_default_formatter.h | ||
| 40–41 | set, map and string. Not all of them now, but they will once the other specializations are implemented. Since it raises questions I've added a comment. | |
| 58 | Good catch, it should have been __get_range_format in the first place. | |
| libcxx/include/__type_traits/is_specialization.h | ||
| 36–40 | I will do that before committing. Adding the trait without a use-case also looks odd. | |
Comment Actions
Looks pretty good to me, just a few comments.
| libcxx/include/__format/concepts.h | ||
|---|---|---|
| 63 | Thanks for adding this comment! I would suggest linking to the section of the standard that defines that concept instead of the paper. | |
| 68–70 | Can you make sure to have a test that would fail if we were using the concept from P2165? | |
| libcxx/include/__format/range_default_formatter.h | ||
| 55–56 | If that works, that would be a bit nicer and it would remove the need for the additional type + comment. | |
| 65 | Can you ensure that you have a test with a range where the reference type is the same as the range, without depending on filesystem? You should also keep your current test for filesystem::path. | |
| 84 | FWIW I wouldn't mind if you used an immediately-invoked lambda expression here. Not a request, just a suggestion. | |
| 86–93 | After consideration, I'm fine with this. I'd much rather remove the offending formatter but let's wait for the LWG issue resolution before we do that. | |
| 99 | ||
| libcxx/test/libcxx/type_traits/is_specialization.compile.pass.cpp | ||
| 46 | ||
Comment Actions
Thanks for the review!
| libcxx/include/__format/concepts.h | ||
|---|---|---|
| 68–70 | As discussed I added a TODO for that. | |
| libcxx/include/__format/range_default_formatter.h | ||
| 55–56 | This works. Thanks for the suggestion! | |
| 65 | I added a test to libcxx/test/std/utilities/format/format.range/format.range.fmtkind/format_kind.compile.pass.cpp. | |
| libcxx/include/__type_traits/is_specialization.h | ||
| 36–40 | I discussed with @ldionne and he likes to do it in the same commit. | |