Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Various cleanups: <chrono> #4119

Merged

Conversation

StephanTLavavej
Copy link
Member

Fixes #1805 after 2.5 years. It's about time!

This is inherently a grab-bag PR (sorry) of various cleanups we identified during the rush to complete C++20. I've structured it into a series of fine-grained commits addressing each comment. Most are for the product code, with only a few changes to the test code. (We generally refrain from large-scale cleanups of test code, as it's low-value work and has the potential to disrupt what the tests were trying to do, but these changes are quite simple.) There are also a few cleanups I noticed myself along the way; I've included them here since they're pretty small.

  • Make _THROW a variadic macro so we never need extra parens.
    • Something I noticed myself.
  • Replace decltype(tzdb::zones) and decltype(tzdb::links) with their actual types.
    • These were relics of an early attempt to use special allocators, which was foiled by the Standard's insistence on std::allocator.
  • _Crt_allocator should provide equality.
  • time_zone and time_zone_link's internal constructors should be tagged.
    • We also need to update some test code that was using these internal constructors.
  • Return const char* as string without braces.
  • Extract _Make_unique_tzdb_info().
  • exchange() the contained value instead of the entire optional.
    • We know _Val.has_value() here, so we can directly reach into the optional.
  • Use positive test and early return.
    • Something I noticed myself. The negative test wasn't providing any value here. Additionally, the two cases are different enough that there's no symmetry advantage to keeping the else, so I believe this is clearer with an early return.
  • Drop empty line at the top of _Make_time_point().
  • Consistently say "leap second deletion" in comments.
  • _InIt is always istreambuf_iterator, not an arbitrary input iterator.
    • I decided that making the code specific to istreambuf_iterator was better than just renaming the parameters to _Iter. Accordingly, we can simplify the code because we know that the value_type will be _CharT.
  • Use const auto& to avoid _Ctype typedef.
    • Something I noticed myself. Now that ctype<_CharT> is so short, we can just mention it in-place.
  • 'q' and 'Q' should always be given durations, otherwise there's a logic error in _Is_valid_type.
  • Avoid dead returns after if constexpr.
    • Something I noticed myself. Unlike runtime if-statements where early returns are often desirable, if constexpr should avoid emitting dead code.
  • _Is_any_of_v => is_same_v for one type.
  • Test cleanup: Drop separate res variable used during debugging.
  • Test cleanup: Consistently make invalid variables constexpr.
  • Drop performance note.
    • I don't believe that this comment is worth keeping.
  • Comment why ++_Begin is safe.
  • Use ios_base with _CATCH_IO_.
    • Something I noticed myself. The first parameter of _CATCH_IO_ is used to qualify badbit. It's usually ios_base, but is allowed to vary for headers that are only dragging in <iosfwd>. chrono has the full definition, so it should just say ios_base here. This was the only affected line.

…links)` with their actual types.

Works towards GH 1805. These were relics of an early attempt to use special allocators, which was foiled by the Standard's insistence on `std::allocator`.
Mentioned in GH 1789, works towards GH 1805.
…tors should be tagged.

We also need to update some test code that was using these internal constructors.

Mentioned in GH 1789, works towards GH 1805.
Mentioned in GH 1789, works towards GH 1805.
Mentioned in GH 1789, works towards GH 1805.
…re `optional`.

We know `_Val.has_value()` here, so we can directly reach into the `optional`.

Mentioned in GH 1789, works towards GH 1805.
Not mentioned in GH 1789, just noticed myself. The negative test wasn't providing any value here. Additionally, the two cases are different enough that there's no symmetry advantage to keeping the `else`, so I believe this is clearer with an early `return`.
Mentioned in GH 1789, works towards GH 1805.
…trary input iterator.

I decided that making the code specific to `istreambuf_iterator` was better than just renaming the parameters to `_Iter`. Accordingly, we can simplify the code because we know that the `value_type` will be `_CharT`.

Mentioned in GH 1789, works towards GH 1805.
Now that `ctype<_CharT>` is so short, we can just mention it in-place.

Not mentioned in GH 1789, just noticed myself.
…otherwise there's a logic error in `_Is_valid_type`.

Mentioned in GH 1870, works towards GH 1805.
Unlike runtime `if`-statements where early `return`s are often desirable, `if constexpr` should avoid emitting dead code.

Not mentioned in GH 1870, just noticed myself.
Mentioned in GH 1870, works towards GH 1805.
…ing.

Mentioned in GH 1870, works towards GH 1805.
Mentioned in GH 1870, works towards GH 1805.
I don't believe that this comment is worth keeping.

Mentioned in GH 1870, works towards GH 1805.
Mentioned in GH 1870, works towards GH 1805.
The first parameter of `_CATCH_IO_` is used to qualify `badbit`. It's usually `ios_base`, but is allowed to vary for headers that are only dragging in `<iosfwd>`. `chrono` has the full definition, so it should just say `ios_base` here. This was the only affected line.

Found myself during code review.
@StephanTLavavej StephanTLavavej added enhancement Something can be improved chrono C++20 chrono labels Oct 23, 2023
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner October 23, 2023 16:00
@StephanTLavavej StephanTLavavej self-assigned this Oct 24, 2023
@StephanTLavavej
Copy link
Member Author

I'm a greedy kitty who's speculatively mirroring my own PR to the MSVC-internal repo - please notify me if any further changes are pushed.

#define _RERAISE throw
#define _THROW(x) throw x
#define _RERAISE throw
#define _THROW(...) throw __VA_ARGS__
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pre-existing: Should we wrap () around the arguments in the expansion to avert weirdness?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be reasonable; throw has extremely low precedence but that is conventional for macros. I'll do this in a followup.

@@ -475,7 +475,7 @@ class _CRTIMP2_PURE_IMPORT _EmptyLockit { // empty lock class used for bin compa
#endif

#define _RERAISE
#define _THROW(x) x._Raise()
#define _THROW(...) __VA_ARGS__._Raise()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto (__VA_ARGS__)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto I'll add this to my next batch of cleanups. Thanks!

@StephanTLavavej StephanTLavavej merged commit f392449 into microsoft:main Oct 26, 2023
37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chrono C++20 chrono enhancement Something can be improved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

<chrono>: Perform cleanups identified during code review
2 participants