Skip to content

Conversation

@StephanTLavavej
Copy link
Member

@StephanTLavavej StephanTLavavej commented Apr 17, 2021

  • Adjust spacing in _Has_ok.
  • Avoid undefined behavior 🙀 when accessing _Last_day_table. _Month could be bogus, so we need to mask the value.
  • Remove unnecessary parentheses.
  • Instead of telling _Chrono_formatter whether to _Allow_precision, teach its _Parse to determine that by inspecting _Ty. This avoids potential mistakes, and allows us to customize the format_error message. (We need separate cases for integral-duration and non-duration, due to the need to inspect typename _Ty::rep.)
  • Add a constructor to _Chrono_formatter so we can provide a time zone abbreviation, instead of setting it later. This slightly reduces verbosity in the formatters below. (Eventually, _Chrono_formatter could probably lock down its access control instead of having public data members, but I'm not changing that here.)
  • Add newlines between non-chained if-statements.
  • Style: Rearrange the hh_mm_ss range check to follow the number line (most negative on the left).
  • Rephrase the hh_mm_ss format_error: exact -24h and 24h are errors.
  • Provide a data member initializer for _Time_zone_abbreviation{};. This isn't necessary, but makes it clear that we haven't forgotten about it.
  • Use constexpr offsets when formatting tai_time and gps_time.
    • This follows the Standard's depicted code (except that it avoids UDLs, which are defined at the bottom of <chrono> and I didn't want to bother with moving them).
    • I believe this is slightly more readable, and it's more efficient for debug codegen: the offset is computed at compile time with no function calls.

@StephanTLavavej StephanTLavavej added bug Something isn't working chrono C++20 chrono labels Apr 17, 2021
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner April 17, 2021 02:15
Co-authored-by: MattStephanson <68978048+MattStephanson@users.noreply.github.com>
@StephanTLavavej StephanTLavavej merged commit 6c06f23 into microsoft:chronat2 Apr 17, 2021
@StephanTLavavej StephanTLavavej deleted the chronat2_fix_ub branch April 17, 2021 05:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working chrono C++20 chrono

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants