`<chrono>` Formatting: C++20's Final Boss #1870

StephanTLavavej · 2021-04-21T11:26:10Z

This merges the work that @eldakesh-ms and @mnatsuhara have accumulated in our chronat2 feature branch.

Thanks to @statementreply and @MattStephanson for their assistance, and to everyone who worked on the previous <chrono> PRs #1341, #323, and #1789 (including @SuperWig and @d-winsor). Also thanks to @barcharcraz and everyone who worked on the <format> PRs #1821 and #1834, plus more in flight.

Resolves #12. Completes C++20.

You stand at the entrance to the Fortress of Chronat. Unlike the last boss's fortress, which extended to infinity in two directions, this fortress seems eternal instead of infinite. Some of it appears to have been constructed in 1970, but there are moss-covered stones from 1600, and some columns are made of a metal-plastic alloy that hasn't been invented yet. Before you reach for the door's handle, it opens smoothly, as if it knew when you would arrive.

Inside, there is only a desk with a cat-a-day calendar, a ticking metronome, and a globe. A hollow voice booms: "Fool! You think to challenge me? I am more than human. More than machinery. I am a specification, and I am everywhere and everywhen. My satellites in orbit have continuously tracked you here. My atomic clocks recorded when you entered this world - and when you will leave. How could you hope to defeat time itself? Indeed, you are too late, as I was Standardized months ago!"

You grit your teeth, ready your feature branch, and charge forward with a shout: "Your time is up!"

For this (time-critical) review, note that there are a number of cards in the To Do column of the Chrono Project tracking minor bugs that will be converted to issues, such as compile-time errors with extreme ratios like duration<long long, femto>, incorrect printing of INT64_MIN, precision handling in %S and %Q, and so forth. We expect to fix these in following PRs for VS 2019 16.10 Preview 4 and beyond, but we don't want to delay this feature shipping in Preview 3.

We have about a day to fix any problems found during final review. Severe bugs in common scenarios are the most important to find. Anything that impacts ABI is also important to identify (although we will hopefully have some time between completing C++20 and locking down its ABI, see #1814). If issues aren't very severe, and especially if they are time-consuming to investigate and resolve, we'll file tracking issues. Similarly for comments about possible added test coverage or cleanups (see #1805 for an example).

Co-authored-by: Elnar Dakeshov <55715127+eldakesh-ms@users.noreply.github.com> Co-authored-by: mnatsuhara <46756417+mnatsuhara@users.noreply.github.com>

* <chronat>: Add year and year_month_day formatting 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. * Clang test * Finesse the tests * review * Comment update Co-authored-by: mnatsuhara <46756417+mnatsuhara@users.noreply.github.com> * PR fixes Co-authored-by: mnatsuhara <46756417+mnatsuhara@users.noreply.github.com>

This follows N4885 [tab:time.format.spec]'s order, and adds {'z', _EO_mod} which was missing.

* Mark _Fill_tm() as _NODISCARD. * Teach _Fill_tm() to handle year_month_day_MEOW by directly extracting components when possible, and constructing a year_month_day{} temporary only when necessary. * Replace all of the operator<<() implementations for calendrical types with their "Effects Equivalent To" implementations from the Standard (modified to follow our conventions and actually compile). This should handle the setw(8) case mentioned by P1361R2 section 6 item 5, whereas implementing operator<<() by calling other operator<<() is (apparently) doomed. * Note that each operator<<() won't compile until all of the formatters that it depends on have been implemented. * Teach _Is_valid_type() to accept year_month_day_MEOW. * Now that _Fill_tm() centralizes the "decompose a calendrical type into its components" logic, we can further centralize the formatter definitions with _Fill_tm_formatter. Unlike the operator<<() overloads (where I added all types), I'm not adding formatters for unimplemented types here. When they're implemented, they should also be able to use _Fill_tm_formatter, unless they need unusual processing beyond what _Fill_tm() provides. * In P0355R7_calendars_and_time_zones_formatting/test.cpp, add placeholder tests, and comment out test_hh_mm_ss_formatter()'s implementation for now.

Adds the formatter with special bounds check so that `put_time` doesn't assert. Small bugfix/simplification for when `ok()` needs to be checked. Before we call `put_time`, we should always bounds check. Custom %S writer added (and %T calls it too). This is necessary for proper fractional second printing.

Not too many tests but they reuse a lot of the same pathways. Wouldn't hurt to test more but the operator<< are fully passthrough to format so it's not too scary.

Thanks to matt and stat on discord for mentioning these edge cases. I originally thought that you can't get a day from `month_day_last`, but it clearly makes sense, for most months. I changed some machinery around so that we can always intercept a specifier, which means that we need to do manual checking in the writer for localization. This PR is more to illustrate how a specifier may be intercepted, and that testing need only be done for specific types.

* chronat: Clock formatting Adds formatting for clocks! Moved _Custom_write into the formatter so it can do special things (write timezones). _Write_seconds now writes leap seconds as 60 for utc clock time points. Taught _Fill_tm to work with time_points (in reality only system_clock and local_clock work). Add operator<< for all clocks except local-time-format-t (because I'm still not sure what that is). The base formatter stores a timezone abbreviation. This is only useful for the clocks, but it seemed like the simplest way to implement this feature. * Remove unnecessary _CharT param for _Custom_write. This is a member function of _Chrono_formatter which is already templated on _CharT. * typename _Ty::clock. Co-authored-by: Stephan T. Lavavej <stl@nuwen.net>

* wd, wdi formatting * Update libcxx skips. * Arrange formatters in Standard order. Co-authored-by: Stephan T. Lavavej <stl@nuwen.net>

* Fix UB, various cleanups. * Optimize with common_type_t<_Duration, days>. Co-authored-by: MattStephanson <68978048+MattStephanson@users.noreply.github.com>

* hh_mm_ss::hours() is already an absolute value. Co-authored-by: statementreply <statementreply@gmail.com> * Implement nonexistent_local_time/ambiguous_local_time. * Test nonexistent_local_time/ambiguous_local_time. * Fix P0355R7_calendars_and_time_zones_time_zones. We can't use floating-point durations because the exception constructors in [time.zone.exception] will stream local_time<Duration>, and [time.clock.local] implements that by streaming sys_time<Duration>, and that's constrained by [time.clock.system.nonmembers] to reject floating-point durations. Co-authored-by: statementreply <statementreply@gmail.com>

…y_last` (microsoft#1854) * Add/test weekday_last, month_weekday, month_weekday_last. * In _Fill_tm(): + weekday_indexed and weekday_last can share code. + month_weekday and month_weekday_last have different accessors. + Cleanup: Unify the code for year_month_weekday and year_month_weekday_last. * In _Is_valid_type(): + weekday, weekday_indexed, and weekday_last all support the "weekday types". + month_weekday and month_weekday_last support "month types" and "weekday types". (As mentioned above, their accessors are actually weekday_indexed() and weekday_last(), but it seemed pointless to have separate cases to "recurse" into the weekday_indexed and weekday_last types, when the answer is always the same.) + Remove TRANSITION and change the final static_assert to "should be unreachable", which is the pattern that we use elsewhere. * Add the new formatters, all powered by _Fill_tm_formatter. In P0355R7_calendars_and_time_zones_formatting/test.cpp: * Rename charT to CharT for consistency (this is needed by the STR macro, if it were ever used in these functions). * Add empty_braces_helper() to test both format("{}") and operator<<. This should supersede stream_helper() but I'm not making that change here. * Test the new types. * Implement tests for year_month_day_last, year_month_weekday, and year_month_weekday_last now that the necessary formatters are available. * Call the new test functions. * Update libcxx skips for C++20 features.

* Rearrange tests to follow Standard order. No other changes. * Move _Chrono_formatter into namespace chrono. No changes other than (greatly reduced!) qualification and formatting. _Fill_tm_formatter is still directly within std, as it's the base class for std::formatter. * Rename _Chrono_specs to _Chrono_spec. Drop "with literal chars" from a comment; this reflected our earlier, incorrect understanding. This also renames _Custom_write()'s parameter from _Specs to _Spec, avoiding shadowing a data member.

* Replace stream_helper with empty_braces_helper. * Replace assert(format(STR("{}"), A) == B) with empty_braces_helper(A, B). * Remove duplicate lines. * Simplify choose_literal, use STR consistently.

* chronat: Add duration formatter Special specifiers for duration are `j q Q`. Otherwise duration is very similar to `hh_mm_ss` except that times are interpreted as time from midnight. I thought this would mean that negative times are yesterday, but we just append a `-` instead, which means we should round instead of flooring to a day when computing hh/mm/ss. Other behavior is pretty simple. * Add typename. * Comments Co-authored-by: Stephan T. Lavavej <stl@nuwen.net>

…icrosoft#1860) * Define and test the feature-test macro. * Implement and test sys_info and local_info. * Update libcxx skips. * Additionally test zero and half-hour offsets. * Print `seconds offset` and `minutes save`. * Add TRANSITION comment.

stl/inc/chrono

eldakesh-ms

Took a structural look. I am really shaky on my understanding of leap seconds and the different clocks.

stl/inc/chrono

Co-authored-by: Stephan T. Lavavej <stl@nuwen.net> Co-authored-by: mnatsuhara <46756417+mnatsuhara@users.noreply.github.com>

stl/inc/chrono

mnatsuhara

Most are smaller comments that are not important enough to reset testing -- I think the only potentially substantial comment is about error-handling for %q and %Q.

mnatsuhara · 2021-04-22T05:49:00Z

stl/inc/chrono


-        _CharT _Mod = '\0';
-        _CharT _Ch  = *_Begin;
+    _CharT _Type = *_Begin;


Very minor, but the _Type member is now of type char, not _CharT.

Looking at this now, I believe that the code is correct. _On_conversion_spec does a range check before storing this as a char:

STL/stl/inc/chrono

Lines 4788 to 4800 in adea8d5

constexpr void _On_conversion_spec(char _Modifier, _CharT _Type) {

// NOTE: same performance note from _Basic_format_specs also applies here

if (_Modifier != '\0' && _Modifier != 'E' && _Modifier != 'O') {

_Throw_format_error("Invalid modifier specification.");

}

if (_Type < 0 || _Type > (numeric_limits<signed char>::max)()) {

_Throw_format_error("Invalid type specification.");

}

_Chrono_spec<_CharT> _Conv_spec{._Modifier = _Modifier, ._Type = static_cast<char>(_Type)};

_Specs._Chrono_specs_list.push_back(_Conv_spec);

}

mnatsuhara · 2021-04-22T19:19:52Z

stl/inc/chrono

+            case 'q':
+                if constexpr (_Is_specialization_v<_Ty, duration>) {
+                    _Write_unit_suffix<typename _Ty::period>(_Os);
+                }
+                return true;
+            case 'Q':
+                if constexpr (_Is_specialization_v<_Ty, duration>) {
+                    _Os << _STD abs(_Val.count());
+                }


Do we need error-handling here? If the given type is not a specialization of duration?

This pattern is pretty common. I wish we could make it more obvious but Q is only valid for duration (same with q), so any type-specifier mismatch should be caught by _Is_valid_type.

I suppose the way we could express this is: if constexpr (types we expect) { /*...*/ } else { _STL_INTERNAL_CHECK(false); } so if the tests ever reach the else we'll see a failure.

mnatsuhara · 2021-04-22T19:33:07Z

stl/inc/chrono

+                case 'Y':
+                    if constexpr (is_same_v<_Ty, year>) {
+                        return _Val.ok();
+                    } else if constexpr (_Is_any_of_v<_Ty, year_month> || _Is_ymd) {


Should we use is_same_v<_Ty, year_month> since we're only looking for year_month in the left part of this test?

I used to enumerate all the year_month_day* types, but then made it a shorthand, so this is a carryover of that.

mnatsuhara · 2021-04-22T19:48:20Z

tests/std/tests/P0355R7_calendars_and_time_zones_formatting/test.cpp

@@ -71,8 +66,8 @@ struct testing_callbacks {
    void _On_dynamic_precision(_Auto_id_tag) {
        assert(expected_auto_dynamic_precision);
    }
-    void _On_conversion_spec(CharT mod, CharT type) {
-        assert(static_cast<char>(mod) == expected_chrono_specs[curr_index]._Modifier);
+    void _On_conversion_spec(char mod, CharT type) {


Since both _Mod and _Type of _Chrono_spec are now char, not _CharT, I think both parameters can be of type char.

Similar to the other comment, I believe this will be called with CharT, so it should take that and then internally perform narrowing, so I'll leave this as-is.

mnatsuhara · 2021-04-22T19:54:03Z

tests/std/tests/P0355R7_calendars_and_time_zones_formatting/test.cpp

+    // 2 digits
+    day d0{27};
+    auto res = format(s0, d0);
+    assert(res == a0);
+    res = format(s1, d0);
+    assert(res == a0);
+
+    // 1 digit
+    day d1{5};
+    res = format(s0, d1);
+    assert(res == a1);
+    res = format(s1, d1);
+    assert(res == a2);
+
+    // O modifier
+    res = format(s2, d0);
+    assert(res == a0);
+    res = format(s3, d0);
+    assert(res == a0);
+    res = format(s2, d1);
+    assert(res == a1);
+    res = format(s3, d1);
+    assert(res == a2);
+
+    // [time.format]/6
+    day d2{50};
+    res = format(s4, d0);
+    assert(res == a0);
+    res = format(s4, d2);
+    assert(res == a3);
+
+    // width/align
+    res = format(s5, d0);
+    assert(res == a4);
+    res = format(s5, d1);
+    assert(res == a5);
+    res = format(s5, d2);
+    assert(res == a3);
+
+    // chrono-spec must begin with conversion-spec
+    throw_helper(s6, d0);
+
+    // lit chars
+    res = format(s7, d0);
+    assert(res == a7);
+    res = format(s8, d0);
+    assert(res == a8);


If we're pushing changes anyway, we can shorten these tests to have the format call be in the assert -- they were split out into the separate res variable originally so I could print out the results as I de-🐞ed

mnatsuhara · 2021-04-22T19:55:01Z

tests/std/tests/P0355R7_calendars_and_time_zones_formatting/test.cpp

+
+template <typename CharT>
+void test_weekday_last_formatter() {
+    constexpr weekday_last invalid{weekday{10}};


Should we make our other invalid types in each function constexpr as well? Again, not worth resetting testing for.

CaseyCarter

Nothing serious; I'll copy these comments into a followup issue and mark them resolved.

tests/libcxx/expected_results.txt

stl/inc/chrono

CaseyCarter · 2021-04-22T20:48:05Z

stl/inc/chrono

-    };
-    // clang-format on
+template <class _CharT>
+_NODISCARD constexpr const _CharT* _Choose_literal(const char* const _Str, const wchar_t* const _WStr) noexcept {


Followup: Would consteval here result in fewer unused string literals emitted into object files?

That's a good idea. I see no reason to avoid it other than caution around compiler bugs.

⚠️ There were compiler bugs - one in MSVC involving conditional expressions (I believe it has been reported) and another in Clang (not yet reduced). I don't think this is worth the effort right now, abandoning the attempt.

stl/inc/chrono

CaseyCarter · 2021-04-22T21:20:39Z

stl/inc/chrono

+                break;
+            }
+        } else { // literal-char
+            _Callbacks._On_lit_char(*_Begin);
            ++_Begin;


Followup: we should comment here that we believe % isn't used as a non-lead-byte in any supported multibyte encoding, so it's safe to use ++ here rather than the _Fmt_codec machinery.

⚠️ This else branch is being used when we aren't looking at %, so I'm unsure if this code has a subtle bug.

StephanTLavavej · 2021-04-23T00:07:46Z

Thanks everyone - we're C++20 feature complete! 😻 🎉 🚀

StephanTLavavej and others added 30 commits April 15, 2021 01:44

Squashed chronat changes.

2118d1e

Co-authored-by: Elnar Dakeshov <55715127+eldakesh-ms@users.noreply.github.com> Co-authored-by: mnatsuhara <46756417+mnatsuhara@users.noreply.github.com>

Fix tests.

33e3804

charT -> char mod/type, small bugfix and nits

cdfa88c

Add hh_mm_ss operator<<

206727e

Fix tests

1cb654b

ymd has info for weekday too

e7b705a

add test for u w

5505ea3

Various cleanups. (microsoft#1842)

4c236a3

Modifier table update

5bf0c50

Update the modifier table, use uint8_t.

1187292

This follows N4885 [tab:time.format.spec]'s order, and adds {'z', _EO_mod} which was missing.

Robuster OOB handling

838cd1f

Add general time filler

3a19af7

Remove failed type checks

889fe20

Update libcxx skips.

48eb7f8

Add month_day, month_day_last, and year_month formatting

29b7094

Not too many tests but they reuse a lot of the same pathways. Wouldn't hurt to test more but the operator<< are fully passthrough to format so it's not too scary.

Expand functionality of _Try_simple_write into _Custom_write

f6ba00d

wd, wdi formatting (microsoft#1847)

d418366

* wd, wdi formatting * Update libcxx skips. * Arrange formatters in Standard order. Co-authored-by: Stephan T. Lavavej <stl@nuwen.net>

Only throw on hh_mm_ss if the hour field is used and is OOB

8aeda0c

<chrono> formatting: fix UB, various cleanups (microsoft#1848)

6c06f23

* Fix UB, various cleanups. * Optimize with common_type_t<_Duration, days>. Co-authored-by: MattStephanson <68978048+MattStephanson@users.noreply.github.com>

<chrono> formatting: Simplify test (microsoft#1859)

c06416c

* Replace stream_helper with empty_braces_helper. * Replace assert(format(STR("{}"), A) == B) with empty_braces_helper(A, B). * Remove duplicate lines. * Simplify choose_literal, use STR consistently.

StephanTLavavej added cxx20 C++20 feature high priority Important! chrono C++20 chrono labels Apr 21, 2021

StephanTLavavej requested a review from a team as a code owner April 21, 2021 11:26

statementreply reviewed Apr 21, 2021

View reviewed changes

stl/inc/chrono Show resolved Hide resolved

stl/inc/chrono Show resolved Hide resolved

stl/inc/chrono Show resolved Hide resolved

stl/inc/chrono Outdated Show resolved Hide resolved

eldakesh-ms reviewed Apr 21, 2021

View reviewed changes

stl/inc/chrono Show resolved Hide resolved

stl/inc/chrono Show resolved Hide resolved

eldakesh-ms reviewed Apr 21, 2021

View reviewed changes

stl/inc/chrono Show resolved Hide resolved

stl/inc/chrono Outdated Show resolved Hide resolved

MattStephanson mentioned this pull request Apr 21, 2021

<chrono> Fixes two-digit year formatting for negative centuries. #1872

Merged

eldakesh-ms and others added 4 commits April 21, 2021 16:42

<chrono>: Fine grained bounds checking (microsoft#1871)

d652eab

Co-authored-by: Stephan T. Lavavej <stl@nuwen.net> Co-authored-by: mnatsuhara <46756417+mnatsuhara@users.noreply.github.com>

Fixes two-digit year for negative centuries. (microsoft#1872)

360e631

Use run_tz_test to catch exceptions and handle internal machines.

bd5b397

Validate year_month::ok() for %g %G.

7c177d0

StephanTLavavej commented Apr 22, 2021

View reviewed changes

stl/inc/chrono Outdated Show resolved Hide resolved

stl/inc/chrono Outdated Show resolved Hide resolved

We don't need _No_chrono_specs.

ab1001c

mnatsuhara approved these changes Apr 22, 2021

View reviewed changes

StephanTLavavej mentioned this pull request Apr 22, 2021

<chrono>: Perform cleanups identified during code review #1805

Closed

CaseyCarter approved these changes Apr 22, 2021

View reviewed changes

CaseyCarter mentioned this pull request Apr 22, 2021

<chronat>: Perform cleanup as noted during code review #1873

Closed

StephanTLavavej merged commit c33874c into microsoft:main Apr 23, 2021

StephanTLavavej deleted the merge_chronat2 branch April 23, 2021 00:05

This was referenced May 4, 2021

<chrono>: file_clock also needs leap second handling #1899

Closed

<chrono>: Reduce put_time calls in [time.format] #1900

Open

kobykahane mentioned this pull request May 18, 2021

<chrono>: Including header pulls in a dynamic initializer, incompatible with /kernel #1926

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`<chrono>` Formatting: C++20's Final Boss #1870

`<chrono>` Formatting: C++20's Final Boss #1870

StephanTLavavej commented Apr 21, 2021

eldakesh-ms left a comment

mnatsuhara left a comment

mnatsuhara Apr 22, 2021

StephanTLavavej Oct 21, 2023

mnatsuhara Apr 22, 2021

eldakesh-ms Apr 22, 2021 •

edited

Loading

StephanTLavavej Apr 22, 2021

mnatsuhara Apr 22, 2021

eldakesh-ms Apr 22, 2021

mnatsuhara Apr 22, 2021

StephanTLavavej Oct 21, 2023

mnatsuhara Apr 22, 2021

mnatsuhara Apr 22, 2021

CaseyCarter left a comment

CaseyCarter Apr 22, 2021

StephanTLavavej Apr 22, 2021

StephanTLavavej Oct 21, 2023

CaseyCarter Apr 22, 2021

StephanTLavavej Oct 21, 2023

StephanTLavavej commented Apr 23, 2021

	constexpr void _On_conversion_spec(char _Modifier, _CharT _Type) {
	// NOTE: same performance note from _Basic_format_specs also applies here
	if (_Modifier != '\0' && _Modifier != 'E' && _Modifier != 'O') {
	_Throw_format_error("Invalid modifier specification.");
	}

	if (_Type < 0 \|\| _Type > (numeric_limits<signed char>::max)()) {
	_Throw_format_error("Invalid type specification.");
	}

	_Chrono_spec<_CharT> _Conv_spec{._Modifier = _Modifier, ._Type = static_cast<char>(_Type)};
	_Specs._Chrono_specs_list.push_back(_Conv_spec);
	}

<chrono> Formatting: C++20's Final Boss #1870

<chrono> Formatting: C++20's Final Boss #1870

Conversation

StephanTLavavej commented Apr 21, 2021

eldakesh-ms left a comment

Choose a reason for hiding this comment

mnatsuhara left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eldakesh-ms Apr 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CaseyCarter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephanTLavavej commented Apr 23, 2021

`<chrono>` Formatting: C++20's Final Boss #1870

`<chrono>` Formatting: C++20's Final Boss #1870

eldakesh-ms Apr 22, 2021 •

edited

Loading