From a26bfeb5c4f910332c5e1afa0908d6400ce8966a Mon Sep 17 00:00:00 2001 From: Elnar D Date: Wed, 21 Apr 2021 08:53:25 -0700 Subject: [PATCH 1/4] : Fine grained bounds checking The motivation of this PR is to loosen some specifier constraints by only checking relavent portions of types instead of a blanket `ok()` before attempting to print. This should cover all the specifiers. I've also moved check-only custom prints into this new method. --- stl/inc/chrono | 187 +++++++++++++----- .../test.cpp | 10 + 2 files changed, 145 insertions(+), 52 deletions(-) diff --git a/stl/inc/chrono b/stl/inc/chrono index e8af360e741..ef4c93fc84a 100644 --- a/stl/inc/chrono +++ b/stl/inc/chrono @@ -5965,13 +5965,8 @@ namespace chrono { if (_Custom_write(_Stream, _Spec, _Time, _Val)) { continue; } - // Otherwise, we should throw on out-of-bounds to avoid triggering asserts within put_time - // machinery. - if constexpr (_Has_ok<_Ty>) { - if (!_Val.ok()) { - _THROW(format_error("Cannot localize out-of-bounds time point.")); - } - } + + _Validate_specifiers(_Spec, _Val); _Stream << _STD put_time<_CharT>(&_Time, _Fmt_string(_Spec).data()); } @@ -5998,29 +5993,16 @@ namespace chrono { const auto _Month = _Time.tm_mon + 1; const bool _Has_modifier = _Spec._Modifier != '\0'; switch (_Spec._Type) { - case 'a': - case 'A': - case 'u': - case 'w': - if constexpr (_Is_any_of_v<_Ty, year_month_weekday, year_month_weekday_last>) { - if (!_Val.weekday().ok()) { - _THROW(format_error("Cannot print invalid weekday")); - } - _Os << _STD put_time(&_Time, _Fmt_string(_Spec).data()); - return true; - } else if constexpr (_Has_ok<_Ty>) { - if (!_Val.ok()) { - _THROW(format_error("Cannot print invalid weekday")); - } - } - return false; - case 'd': + case 'd': // Print days as a decimal, even if invalid. case 'e': // Most months have a proper last day, but February depends on the year. if constexpr (is_same_v<_Ty, month_day_last>) { if (_Val.month() == February) { _THROW(format_error("Cannot print the last day of February without a year")); } + if (!_Val.ok()) { + return false; + } } if (_Has_modifier) { @@ -6063,20 +6045,6 @@ namespace chrono { if constexpr (_Is_specialization_v<_Ty, duration>) { _Os << _STD abs(_CHRONO duration_cast(_Val).count()); return true; - } else if constexpr (is_same_v<_Ty, month_day>) { - if (_Val.month() > February) { - _THROW(format_error("The day of year for a month_day past February is ambiguous.")); - } - } else if constexpr (is_same_v<_Ty, month_day_last>) { - if (_Val.month() >= February) { - _THROW(format_error("The day of year for a month_day_last other than January is ambiguous")); - } - } - - if constexpr (_Has_ok<_Ty>) { - if (!_Val.ok()) { - _THROW(format_error("Cannot print invalid day of year")); - } } return false; case 'q': @@ -6089,7 +6057,7 @@ namespace chrono { _Os << _STD abs(_Val.count()); } return true; - case 'm': + case 'm': // Print months as a decimal, even if invalid. if (_Has_modifier) { return false; } @@ -6099,7 +6067,7 @@ namespace chrono { } _Os << _Month; return true; - case 'Y': + case 'Y': // Print years as a decimal, even if invalid. if (_Has_modifier) { return false; } @@ -6109,13 +6077,13 @@ namespace chrono { } _Os << _STD format(_STATICALLY_WIDEN(_CharT, "{:04}"), _STD abs(_Year)); return true; - case 'y': + case 'y': // Print the years as a decimal, even if invalid. if (_Has_modifier) { return false; } _Os << _STD format(_STATICALLY_WIDEN(_CharT, "{:02}"), _Year < 0 ? 100 + (_Year % 100) : _Year % 100); return true; - case 'C': + case 'C': // Print the years as a decimal, even if invalid. if (_Has_modifier) { return false; } @@ -6126,30 +6094,23 @@ namespace chrono { _Os << _STD format(_STATICALLY_WIDEN(_CharT, "{:02}"), _STD abs(_Time_parse_fields::_Decompose_year(_Year).first) / 100); return true; - case 'F': + case 'F': // Print YMD even if invalid. _Custom_write(_Os, {._Type = 'Y'}, _Time, _Val); _Os << _CharT{'-'}; _Custom_write(_Os, {._Type = 'm'}, _Time, _Val); _Os << _CharT{'-'}; _Custom_write(_Os, {._Type = 'd'}, _Time, _Val); return true; - case 'D': + case 'D': // Print YMD even if invalid. _Custom_write(_Os, {._Type = 'm'}, _Time, _Val); _Os << _CharT{'/'}; _Custom_write(_Os, {._Type = 'd'}, _Time, _Val); _Os << _CharT{'/'}; _Custom_write(_Os, {._Type = 'y'}, _Time, _Val); return true; - case 'H': - if constexpr (_Is_specialization_v<_Ty, hh_mm_ss>) { - if (_Val.hours() >= hours{24}) { - _THROW(format_error("Cannot localize hh_mm_ss with an absolute value of 24 hours or more.")); - } - } - return false; case 'T': // Alias for %H:%M:%S but we need to rewrite %S to display fractions of a second. - _Custom_write(_Os, {._Type = 'H'}, _Time, _Val); + _Validate_specifiers({._Type = 'H'}, _Val); _Os << _STD put_time(&_Time, _STATICALLY_WIDEN(_CharT, "%H:%M:")); [[fallthrough]]; case 'S': @@ -6202,6 +6163,128 @@ namespace chrono { } } + template + void _Validate_specifiers(const _Chrono_spec<_CharT>& _Spec, const _Ty& _Val) { + // clang-format off + if constexpr ( + _Is_specialization_v<_Ty, duration> + || is_same_v<_Ty, sys_info> || _Is_specialization_v<_Ty, time_point> + || _Is_specialization_v<_Ty, _Local_time_format_t>) { + return; + } + // clang-format on + + if constexpr (_Is_specialization_v<_Ty, hh_mm_ss>) { + if (_Spec._Type == 'H' && _Val.hours() >= hours{24}) { + _THROW(format_error("Cannot localize hh_mm_ss with an absolute value of 24 hours or more.")); + } + return; + } + + constexpr bool _Is_ymd = + _Is_any_of_v<_Ty, year_month_day, year_month_day_last, year_month_weekday, year_month_weekday_last>; + + const auto _Validate = [&] { + switch (_Spec._Type) { + case 'a': + case 'A': + case 'u': + case 'w': + if constexpr (_Is_any_of_v<_Ty, weekday, weekday_last>) { + return _Val.ok(); + } else if constexpr (_Is_any_of_v<_Ty, weekday_indexed, year_month_weekday, + year_month_weekday_last>) { + return _Val.weekday().ok(); + } else if constexpr (is_same_v<_Ty, month_weekday>) { + return _Val.weekday_indexed().weekday().ok(); + } else if constexpr (is_same_v<_Ty, month_weekday_last>) { + return _Val.weekday_last().ok(); + } else if constexpr (_Is_any_of_v<_Ty, year_month_day, year_month_day_last>) { + return _Val.ok(); + } + break; + + case 'b': + case 'B': + case 'h': + case 'm': + if constexpr (is_same_v<_Ty, month>) { + return _Val.ok(); + } else if constexpr (_Is_any_of_v<_Ty, month_day, month_day_last, month_weekday, month_weekday_last, + year_month> || _Is_ymd) { + return _Val.month().ok(); + } + break; + + case 'C': + case 'y': + case 'Y': + if constexpr (is_same_v<_Ty, year>) { + return _Val.ok(); + } else if constexpr (_Is_any_of_v<_Ty, year_month> || _Is_ymd) { + return _Val.year().ok(); + } + break; + + case 'd': + case 'e': + if constexpr (_Is_any_of_v<_Ty, day, month_day_last>) { + return _Val.ok(); + } else if constexpr (is_same_v<_Ty, month_day>) { + return _Val.day().ok(); + } else if constexpr (_Is_ymd) { + const year_month_day& _Ymd{_Val}; + return _Ymd.day().ok(); + } + break; + + case 'D': + case 'F': + if constexpr (_Has_ok<_Ty>) { + return _Val.ok(); + } + break; + + case 'j': + if constexpr (is_same_v<_Ty, month_day>) { + if (_Val.month() > February) { + _THROW(format_error("The day of year for a month_day past February is ambiguous.")); + } + return true; + } else if constexpr (is_same_v<_Ty, month_day_last>) { + if (_Val.month() >= February) { + _THROW( + format_error("The day of year for a month_day_last other than January is ambiguous")); + } + return true; + } else if constexpr (_Is_ymd) { + return _Val.ok(); + } + break; + + case 'g': + case 'G': + case 'U': + case 'V': + case 'W': + if constexpr (_Is_ymd) { + return _Val.ok(); + } + + default: + if constexpr (_Has_ok<_Ty>) { + return _Val.ok(); + } + return true; + } + _STL_INTERNAL_CHECK(false); + return false; + }; + if (!_Validate()) { + _THROW(format_error("Cannot localize out-of-bounds time point.")); + } + } + _NODISCARD static array<_CharT, 4> _Fmt_string(const _Chrono_spec<_CharT>& _Spec) { array<_CharT, 4> _Fmt_str; size_t _Next_idx = 0; diff --git a/tests/std/tests/P0355R7_calendars_and_time_zones_formatting/test.cpp b/tests/std/tests/P0355R7_calendars_and_time_zones_formatting/test.cpp index 6fd4b2117b9..0fbda731df3 100644 --- a/tests/std/tests/P0355R7_calendars_and_time_zones_formatting/test.cpp +++ b/tests/std/tests/P0355R7_calendars_and_time_zones_formatting/test.cpp @@ -444,6 +444,7 @@ void test_weekday_indexed_formatter() { assert(format(STR("{:%a %A}"), weekday_indexed{Monday, 2}) == STR("Mon Monday")); assert(format(STR("{:%u %w}"), weekday_indexed{Tuesday, 3}) == STR("2 2")); assert(format(STR("{:%u %w}"), weekday_indexed{Sunday, 4}) == STR("7 0")); + assert(format(STR("{:%u %w}"), weekday_indexed{Sunday, 10}) == STR("7 0")); } template @@ -464,6 +465,8 @@ void test_month_day_formatter() { assert(format(STR("{:%B %d}"), June / 17) == STR("June 17")); throw_helper(STR("{:%Y}"), June / 17); + assert(format(STR("{:%B}"), June / day{40}) == STR("June")); + throw_helper(STR("{:%B}"), month{13}); assert(format(STR("{:%j}"), January / 5) == STR("005")); assert(format(STR("{:%j}"), February / 5) == STR("036")); @@ -479,6 +482,7 @@ void test_month_day_last_formatter() { assert(format(STR("{:%B}"), June / last) == STR("June")); assert(format(STR("{:%d}"), June / last) == STR("30")); throw_helper(STR("{:%d}"), February / last); + throw_helper(STR("{:%B}"), month{13} / last); assert(format(STR("{:%j}"), January / last) == STR("031")); throw_helper(STR("{:%j}"), February / last); @@ -505,6 +509,9 @@ void test_month_weekday_formatter() { assert(format(STR("{:%b %B %h %m %a %A %u %w}"), mwd1) == STR("Aug August Aug 08 Tue Tuesday 2 2")); assert(format(STR("{:%b %B %h %m %a %A %u %w}"), mwd2) == STR("Dec December Dec 12 Sun Sunday 7 0")); + + assert(format(STR("{:%u}"), invalid1) == STR("5")); + throw_helper(STR("{:%u}"), invalid2); } template @@ -525,6 +532,9 @@ void test_month_weekday_last_formatter() { assert(format(STR("{:%b %B %h %m %a %A %u %w}"), mwdl1) == STR("Aug August Aug 08 Tue Tuesday 2 2")); assert(format(STR("{:%b %B %h %m %a %A %u %w}"), mwdl2) == STR("Dec December Dec 12 Sun Sunday 7 0")); + + assert(format(STR("{:%u}"), invalid2) == STR("5")); + throw_helper(STR("{:%u}"), invalid1); } template From ff2c5c5b278b0bf06b071ee3fa8344a7460d9554 Mon Sep 17 00:00:00 2001 From: Elnar D Date: Wed, 21 Apr 2021 09:09:02 -0700 Subject: [PATCH 2/4] Break --- stl/inc/chrono | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/stl/inc/chrono b/stl/inc/chrono index ef4c93fc84a..17e635efbd3 100644 --- a/stl/inc/chrono +++ b/stl/inc/chrono @@ -6167,9 +6167,8 @@ namespace chrono { void _Validate_specifiers(const _Chrono_spec<_CharT>& _Spec, const _Ty& _Val) { // clang-format off if constexpr ( - _Is_specialization_v<_Ty, duration> - || is_same_v<_Ty, sys_info> || _Is_specialization_v<_Ty, time_point> - || _Is_specialization_v<_Ty, _Local_time_format_t>) { + _Is_specialization_v<_Ty, duration> || is_same_v<_Ty, sys_info> + || _Is_specialization_v<_Ty, time_point> || _Is_specialization_v<_Ty, _Local_time_format_t>) { return; } // clang-format on @@ -6270,6 +6269,7 @@ namespace chrono { if constexpr (_Is_ymd) { return _Val.ok(); } + break; default: if constexpr (_Has_ok<_Ty>) { From 73f26eda9bc45f0946b01cb0d1f3d84707dda0a6 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Wed, 21 Apr 2021 15:22:56 -0700 Subject: [PATCH 3/4] Code review. Add newline between non-chained if-statements. Clarify comments. _Validate_specifiers can be static. In test_month_day_formatter, verify that a month_day with a bogus month but valid day can't be printed as a month. --- stl/inc/chrono | 7 ++++--- .../P0355R7_calendars_and_time_zones_formatting/test.cpp | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/stl/inc/chrono b/stl/inc/chrono index 17e635efbd3..73660012716 100644 --- a/stl/inc/chrono +++ b/stl/inc/chrono @@ -6000,6 +6000,7 @@ namespace chrono { if (_Val.month() == February) { _THROW(format_error("Cannot print the last day of February without a year")); } + if (!_Val.ok()) { return false; } @@ -6077,13 +6078,13 @@ namespace chrono { } _Os << _STD format(_STATICALLY_WIDEN(_CharT, "{:04}"), _STD abs(_Year)); return true; - case 'y': // Print the years as a decimal, even if invalid. + case 'y': // Print the two-digit year as a decimal, even if invalid. if (_Has_modifier) { return false; } _Os << _STD format(_STATICALLY_WIDEN(_CharT, "{:02}"), _Year < 0 ? 100 + (_Year % 100) : _Year % 100); return true; - case 'C': // Print the years as a decimal, even if invalid. + case 'C': // Print the century as a decimal, even if invalid. if (_Has_modifier) { return false; } @@ -6164,7 +6165,7 @@ namespace chrono { } template - void _Validate_specifiers(const _Chrono_spec<_CharT>& _Spec, const _Ty& _Val) { + static void _Validate_specifiers(const _Chrono_spec<_CharT>& _Spec, const _Ty& _Val) { // clang-format off if constexpr ( _Is_specialization_v<_Ty, duration> || is_same_v<_Ty, sys_info> diff --git a/tests/std/tests/P0355R7_calendars_and_time_zones_formatting/test.cpp b/tests/std/tests/P0355R7_calendars_and_time_zones_formatting/test.cpp index 0fbda731df3..6e97f6bb7c7 100644 --- a/tests/std/tests/P0355R7_calendars_and_time_zones_formatting/test.cpp +++ b/tests/std/tests/P0355R7_calendars_and_time_zones_formatting/test.cpp @@ -466,7 +466,7 @@ void test_month_day_formatter() { assert(format(STR("{:%B %d}"), June / 17) == STR("June 17")); throw_helper(STR("{:%Y}"), June / 17); assert(format(STR("{:%B}"), June / day{40}) == STR("June")); - throw_helper(STR("{:%B}"), month{13}); + throw_helper(STR("{:%B}"), month{13} / 17); assert(format(STR("{:%j}"), January / 5) == STR("005")); assert(format(STR("{:%j}"), February / 5) == STR("036")); From 20560e3ebde4f2afca00422234706dfd7a5a954a Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Wed, 21 Apr 2021 15:52:05 -0700 Subject: [PATCH 4/4] Adjust formatting. Co-authored-by: mnatsuhara <46756417+mnatsuhara@users.noreply.github.com> --- stl/inc/chrono | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/stl/inc/chrono b/stl/inc/chrono index 73660012716..02100ddbd27 100644 --- a/stl/inc/chrono +++ b/stl/inc/chrono @@ -6167,8 +6167,7 @@ namespace chrono { template static void _Validate_specifiers(const _Chrono_spec<_CharT>& _Spec, const _Ty& _Val) { // clang-format off - if constexpr ( - _Is_specialization_v<_Ty, duration> || is_same_v<_Ty, sys_info> + if constexpr (_Is_specialization_v<_Ty, duration> || is_same_v<_Ty, sys_info> || _Is_specialization_v<_Ty, time_point> || _Is_specialization_v<_Ty, _Local_time_format_t>) { return; }