chronat: Add year and year_month_day formatting #1840
Conversation
This one is a biggie. The main things changed are the way that specifiers are handled and delegated. The general idea behind formatting time is that you can take segments of a type and format them individually. For example, you can take the year out of year_month_day and do the exact same operations you can do with a normal year. The `_Is_type_valid` function recursively checks if a parent type can be formatted by its children. The other functions didn't really need much more finessing, the `tm` structure already has all the fields we need to hold all the time info (simultaniously) and the formatters work off that. The big change here is in moving some "basic" formatters into our own function and not relying on `get_time` to format them. The main reason is that `get_time` does not play with invalid ranges, at all. A day of `40` is always illegal, but we need to be able to format it, especially in the face of `operator <<`. We could have kept what we had before, but then it becomes a clear problem that we cannot use `%F` for a `year_month_day` that has an invalid day, so I am seperating all the integral formatters out into that function. Again, because of the nested nature of times, we recurse in this function. Note that function currently uses `format_to` in probably a very inneficient way. I am all ears on how to improve that.
| case 'a': | ||
| case 'A': | ||
| case 'b': | ||
| case 'B': | ||
| case 'h': | ||
| case 'z': | ||
| case 'Z': |
There was a problem hiding this comment.
Do you want to just populate this with all of the type specifiers that will end up here? Or were you planning to add to this as we added formatters/operator<<s for each type? I think we'll also need to include %c, %x, %X looking at Table 101.
There was a problem hiding this comment.
Hmm, I think we need to rename this function. It was conceived as a way to figure out specifiers can throw, so we can explicit about ok() checking. %c uses locale, but I don't think it can ever be out of bounds. Maybe _Type_needs_bounds_checking?
There was a problem hiding this comment.
I think %c will actually require more extensive bounds-checking because put_time [says that %c uses all fields of tm so there is even more opportunity for out-of-bounds issues?
There was a problem hiding this comment.
I want to say that %c is used for a point in time, measured from some base. You can't have an invalid seconds, because you derive the seconds from the amount of time elapsed.
There was a problem hiding this comment.
I'm trying to work out what type would be used to format %c and I think it would probably be time_points right? (e.g., sys_time, etc.). If we would just use time_point::time_since_epoch() then I think you're right.
| return _Type == 'D' || _Type == 'F' || _Is_valid_type<_CHRONO year>(_Type) | ||
| || _Is_valid_type<_CHRONO month>(_Type) || _Is_valid_type<_CHRONO day>(_Type); |
There was a problem hiding this comment.
I'm wondering if the specifiers that are valid for weekday are also valid here since a specific year, month, and weekday would give us a particular weekday. Similarly, would 'U' and 'V' be valid specifiers since we could get a particular week from a year_month_day?
There was a problem hiding this comment.
We do not have a weekday. I based which specifiers are legal from put_time, and since it isn't trivial to compute the weekday from just a year/month/day, I think it doesn't apply. I could be very wrong though.
There was a problem hiding this comment.
After discussing on discord, I agree it makes sense to be able to derive some more information from a year_month_day than I originally implemented.
Co-authored-by: mnatsuhara <46756417+mnatsuhara@users.noreply.github.com>
CaseyCarter
changed the title
tests/std/tests/P0355R7_calendars_and_time_zones_formatting/test.cpp
Outdated
Show resolved
Hide resolved
| assert(format(STR("{}"), year_month_day{year{1900}, month{1}, day{1}}) == STR("1900-01-01")); | ||
| stream_helper(STR("1900-01-01"), year_month_day{year{1900}, month{1}, day{1}}); | ||
| stream_helper(STR("1900-00-01 is not a valid date"), invalid); | ||
|
|
||
| assert(format(STR("{:%Y %b %d}"), year_month_day{year{1900}, month{1}, day{1}}) == STR("1900 Jan 01")); | ||
| assert(format(STR("{:%F %D}"), invalid) == STR("1900-00-01 00/01/00")); |
There was a problem hiding this comment.
An issue with using 1900-01-01 as an example is that it won't detect swapping the month and the day. I recommend using test data where the values are distinct (when possible).
Similarly, invalid printing 00/01/00 wouldn't detect egregious swapping.
This one is a biggie. The main things changed are the way that
specifiers are handled and delegated.
The general idea behind formatting time is that you can take segments of
a type and format them individually. For example, you can take the year
out of year_month_day and do the exact same operations you can do with a
normal year. The
_Is_type_validfunction recursively checks if aparent type can be formatted by its children. The other functions didn't
really need much more finessing, the
tmstructure already has all thefields we need to hold all the time info (simultaniously) and the
formatters work off that.
The big change here is in moving some "basic" formatters into our own
function and not relying on
get_timeto format them. The main reasonis that
get_timedoes not play with invalid ranges, at all. A day of40is always illegal, but we need to be able to format it, especiallyin the face of
operator <<. We could have kept what we had before, butthen it becomes a clear problem that we cannot use
%Ffor ayear_month_daythat has an invalid day, so I am seperating all theintegral formatters out into that function. Again, because of the nested
nature of times, we recurse in this function.
Note that function currently uses
format_toin probably a veryinneficient way. I am all ears on how to improve that.