Skip to content

Conversation

@SuperWig
Copy link
Contributor

@SuperWig SuperWig commented Nov 21, 2019

Description

Partially implemented #12;
[time.point.arithmetic] and [time.cal] (excluding I/O)

I've used a couple of Howard's algorithms, which I hope is okay as it's public domain

Consider these donated to the public domain

I originally used Howard's algorithm for year_­month_­day::operator sys_days() but I just saw LWG3206 in the issue thread and switched to it however it broke my test project's uses of constexpr due to the depth limit. I did eventually realise this was just a result of poor reading comprehension on my end.

Speaking of constexpr I had to disable the warning for negative integral => unsigned as uses of it in constexpr contexts fail to compile. No idea if that was the correct thing to do.

There is more than likely a better way of doing year_month_weekday::operator sys_days() const than how I implemented it. edit: I believe I've improved this.

During testing I found that this code triggers an ICE

constexpr auto test = 2019y / November / 24;
constexpr auto ymwdl =  2019y / November  / weekday_last{ Sunday };
constexpr sys_days _Last{ 2019y / November / last };

I think everything should be noexcept /* strengthened */? If so are they applied to declarations, definitions, or both?

Checklist

Be sure you've read README.md and understand the scope of this repo.

If you're unsure about a box, leave it unchecked. A maintainer will help you.

  • Identifiers in product code changes are properly _Ugly as per
    https://eel.is/c++draft/lex.name#3.1 or there are no product code changes.
  • The STL builds successfully and all tests have passed (must be manually
    verified by an STL maintainer before automated testing is enabled on GitHub,
    leave this unchecked for initial submission).
  • These changes introduce no known ABI breaks (adding members, renaming
    members, adding virtual functions, changing whether a type is an aggregate
    or trivially copyable, etc.).
  • These changes were written from scratch using only this repository,
    the C++ Working Draft (including any cited standards), other WG21 papers
    (excluding reference implementations outside of proposed standard wording),
    and LWG issues as reference material. If they were derived from a project
    that's already listed in NOTICE.txt, that's fine, but please mention it.
    If they were derived from any other project (including Boost and libc++,
    which are not yet listed in NOTICE.txt), you must mention it here,
    so we can determine whether the license is compatible and what else needs
    to be done.

@SuperWig SuperWig requested a review from a team as a code owner November 21, 2019 14:13
@SuperWig
Copy link
Contributor Author

SuperWig commented Nov 21, 2019

Maybe I should have squashed the commits 👀

Edit: decided to squash them as they added a lot of noise to the conversation. Surprised GitHub doesn't collapse them or something.

@StephanTLavavej StephanTLavavej added the cxx20 C++20 feature label Feb 5, 2020
@SuperWig

This comment has been minimized.

@StephanTLavavej

This comment has been minimized.

@azure-pipelines

This comment has been minimized.

@SuperWig

This comment has been minimized.

@StephanTLavavej

This comment has been minimized.

@StephanTLavavej
Copy link
Member

Thanks for implementing a huge part of this huge C++20 feature, and for enduring over a year (!!!) of review! 😻 📅 📆 🚀 🎉

@SuperWig
Copy link
Contributor Author

SuperWig commented Feb 2, 2021

And thanks for enduring my sometimes questionable code 😅.

One thing though, is this one not an "issue"? #323 (comment)

@StephanTLavavej
Copy link
Member

Thanks, missed that outstanding issue - basically, we're not super duper disciplined about our // SHOUTY COMMENT BANNERS, so I think this is fine. It can always be changed later, but I don't think it's a consistency problem as-is. (Basically I would accept a PR making it shoutier, but won't lose sleep if such a PR never appears.) 😹

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cxx20 C++20 feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants