feature/format arg_id parsing, align parsing, width parsing, skeleton classes - #1232
Charlie Barto (barcharcraz) merged 30 commits into
Conversation
Michael Schellenberger Costa (miscco)
left a comment
There was a problem hiding this comment.
This already looks quite good, I only came to basic_format_parse_context until my brain exploded.
| } else if (*_Begin == '{') { | ||
| ++_Begin; | ||
| if(_Begin != _End) { | ||
| _Begin = _Parse_arg_id(_Begin, _End, _Width_adapter(_Callbacks)); |
There was a problem hiding this comment.
I believe we should move this down. If *_Begin == '}' then _Parse_arg_id will early return and we will fail directly after this.
There was a problem hiding this comment.
I don't understand the issue, if begin == } then _Parse_arg_id will parse the ID as an auto-id and call the right handler for that, as it should
Casey Carter (CaseyCarter)
left a comment
There was a problem hiding this comment.
Overall: I'm concerned that there isn't test coverage here for all of the library components we're adding to <format>. Ideally we'd add components and tests at the same time, but // TODO: test coverage comments or something would be fine for this interim branch.
| auto s0 = ""sv; | ||
| auto s2 = "*<"sv; | ||
| auto s3 = "*>"sv; | ||
| auto s4 = "*^"sv; |
There was a problem hiding this comment.
Missing test cases for "}", "{", and "{<".
well that's good since after basic_format_parse_context the parsing bits end and we go into skeleton code that really just looks like the standard edit: actually I'd encourage you to speak up about bits you find confusing, since I found a lot of stuff confusing and want to add appropriate comments to make it easier for the next guy. |
330cbbc to
14f0b50
Compare
Casey Carter (CaseyCarter)
left a comment
There was a problem hiding this comment.
There are a few "outdated but unresolved" comments in my earlier review as well.
| auto s5 = L"*\x343E"sv; | ||
| test_parse_helper(parse_align_fn, s5, false, view_typ::npos, {_Align::_None, L"*"sv}); | ||
| } | ||
|
|
There was a problem hiding this comment.
Why are we checking the end position of the parse for any of the above cases?
There was a problem hiding this comment.
I think you mean "why are we not checking", Casey. You were concerned about the parse potentially running off the end of the input.
Again, I'm happy to merge as-is and clean up later.
Co-authored-by: Casey Carter <cartec69@gmail.com>
Co-authored-by: Casey Carter <cartec69@gmail.com>
Casey Carter (CaseyCarter)
left a comment
There was a problem hiding this comment.
Couple of small things that can wait til later if you like.
| } | ||
| for (;;) { | ||
| switch (*_Align_pt) { | ||
| case L'<': |
There was a problem hiding this comment.
comparing to L'{' here solves the problem where we narrow wide characters and mess up on wide characters where the least significant byte happens to be narrow '{'.
Switching on *_Align_pt instead of static_cast<char>(*_Align_pt) solved that problem. It's a different problem that comparing to L'<' assumes that < in every narrow encoding has the same value as L'<' and/or u8'<'.
[format.string.general]/2, [format.string.std]/2,3
pretty much all parsing functions are constexpr on all paths, but the standard does not
require much of anything in to be constexpr, so it's more of a coding / debugging aid
than anything else. In particular all the number parsing isn't constexpr because it calls out to
from_chars.
Callbacks for parsing functions are specified in concepts _Parse_spec_callbacks (for [format.string.std]/1) and _Parse_arg_id_callbacks (for the arg-id field of the grammar in [format.string.general]/1
This PR has a sprinkling of all over, but future PRs should be more focused (I hope).
Tests included, however they are not complete.