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

Move <ranges> and <format> into C++20 #2518

Merged
merged 13 commits into from
Feb 7, 2022

Conversation

CaseyCarter
Copy link
Member

@CaseyCarter CaseyCarter commented Feb 4, 2022

and add 128-bit signed and unsigned integer-class types to be the difference of iota_view<64-bits>.

  • In <yvals_core.h>, don't require _HAS_CXX23 to define __cpp_lib_format and __cpp_lib_ranges. Change the feature-test macro test consistently.

  • Change env.lst for all tests that touch <ranges> and <format> to use 20 matrices instead of latest.

  • Don't use the span constructor added to C++23 by WG21-P1989 in P0896R4_views_split, which now must run in C++20 mode where that constructor is unavailable.

Fixes #1814.

For ease of review there are two commits: the first is just the 23-to-20 changes, and the second adds the integer-class type.

Internal-only changes

Reminder for MSVC-internal tasks (the "add new internal header" things):

  • Notify vctowner that we're adding a new internal header __msvc_int128.hpp.
  • Run src/vctools/crt/lkgsync/updatelkgmanifest.cmd.
  • Edit src/SetupPackages/swix/crt.headers/files.base.swr.
  • Edit src/vctools/crt/copy_crt/copy_crt.nativeproj.
  • Edit src/qa/VC/FE/compiler/tests/cxx/modules/dependency-scanning/header-units/deps.cpp.
  • Edit src/qa/VC/FE/compiler/tests/cxx/modules/dependency-scanning/header-units/stl-header-units.dat.

* In `<yvals_core.h>`, don't require `_HAS_CXX23` to define `__cpp_lib_format` and `__cpp_lib_ranges`. Change the feature-test macro test consistently.

* Change test matrices for all tests that touch `<ranges>` and `<format>` to run in 20 mode instead of latest-only.

* Don't use the `span` constructor added by P1989 to C++23 in `P0896R4_views_split`, which now must run in c++20 mode where that constructor is unavailable.

Fixes microsoft#1814.
@CaseyCarter CaseyCarter added cxx20 C++20 feature high priority Important! defect report Applied retroactively labels Feb 4, 2022
@CaseyCarter CaseyCarter requested a review from a team as a code owner February 4, 2022 06:21
@AlexGuteniev
Copy link
Contributor

If there's a need to introduce 128-bit integers, maybe put them into an "extended" header, not "internal" header, and address DevCom-879048 ?

@AlexGuteniev
Copy link
Contributor

If there's an issue with iota_view<64-bits> which is resolved be 128-bit integer, what about using this new integer with iota_view<128-bits> 🤔 ?

stl/inc/__msvc_int128.hpp Show resolved Hide resolved
stl/inc/__msvc_int128.hpp Show resolved Hide resolved
@AlexGuteniev
Copy link
Contributor

Can/should this new big integer be merged with big integer from #2366 ?

stl/inc/__msvc_int128.hpp Outdated Show resolved Hide resolved
stl/inc/__msvc_int128.hpp Outdated Show resolved Hide resolved
stl/inc/__msvc_int128.hpp Outdated Show resolved Hide resolved
stl/inc/__msvc_int128.hpp Outdated Show resolved Hide resolved
stl/inc/__msvc_int128.hpp Outdated Show resolved Hide resolved
stl/inc/__msvc_int128.hpp Outdated Show resolved Hide resolved
stl/inc/__msvc_int128.hpp Outdated Show resolved Hide resolved
stl/inc/__msvc_int128.hpp Outdated Show resolved Hide resolved
@StephanTLavavej
Copy link
Member

@AlexGuteniev #2518 (comment)

Can/should this new big integer be merged with big integer from #2366 ?

Good question - but no, _Big_integer_flt is very big (stores many bits), uses 32-bit elements, and is paradoxically very small - it supports an extremely limited set of operations compared to what Casey has implemented here. There's really no commonality.

Copy link
Contributor

@miscco miscco left a comment

Choose a reason for hiding this comment

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

Some nits, feel free to ignore all the debug codegen stuff. We can do this later when time is not pressing

stl/inc/__msvc_int128.hpp Outdated Show resolved Hide resolved
stl/inc/__msvc_int128.hpp Outdated Show resolved Hide resolved
stl/inc/__msvc_int128.hpp Show resolved Hide resolved
stl/inc/__msvc_int128.hpp Show resolved Hide resolved
stl/inc/__msvc_int128.hpp Outdated Show resolved Hide resolved
stl/inc/__msvc_int128.hpp Outdated Show resolved Hide resolved
stl/inc/__msvc_int128.hpp Outdated Show resolved Hide resolved
stl/inc/__msvc_int128.hpp Show resolved Hide resolved
stl/inc/__msvc_int128.hpp Show resolved Hide resolved
stl/inc/__msvc_int128.hpp Outdated Show resolved Hide resolved
@CaseyCarter
Copy link
Member Author

CaseyCarter commented Feb 5, 2022

If there's a need to introduce 128-bit integers, maybe put them into an "extended" header, not "internal" header, and address DevCom-879048 ?

The type exists and is available if people want to use it, albeit with the hard-to-spell name iter_difference_t<ranges::iterator_t<ranges::iota_view<long long>>>. I don't think we want to advertise it as the type those folks want, at least not until we've addressed the performance concerns and stressed it for a while to make sure the bugs are out.

If there's an issue with iota_view<64-bits> which is resolved be 128-bit integer, what about using this new integer with iota_view<128-bits> 🤔 ?

The goal of the change isn't to require the library to provide arbitrary-precision integers, its to ensure that people don't stumble into UB using iota_view<long long> and its iterators normally, e.g., ranges::distance(views::iota(numeric_limits<long long>::min(), numeric_limits<long long>::max())). See WG21-P1522 for gory detail.

stl/inc/__msvc_int128.hpp Outdated Show resolved Hide resolved
stl/inc/__msvc_int128.hpp Outdated Show resolved Hide resolved
tests/std/tests/P1522R1_difference_type/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/P1522R1_difference_type/test.cpp Outdated Show resolved Hide resolved
stl/inc/__msvc_int128.hpp Show resolved Hide resolved
stl/inc/__msvc_int128.hpp Outdated Show resolved Hide resolved
stl/inc/__msvc_int128.hpp Outdated Show resolved Hide resolved
tests/std/tests/P1522R1_difference_type/test.cpp Outdated Show resolved Hide resolved
stl/inc/__msvc_int128.hpp Outdated Show resolved Hide resolved
stl/inc/__msvc_int128.hpp Outdated Show resolved Hide resolved
stl/inc/__msvc_int128.hpp Outdated Show resolved Hide resolved
stl/inc/__msvc_int128.hpp Outdated Show resolved Hide resolved
stl/inc/__msvc_int128.hpp Outdated Show resolved Hide resolved
stl/inc/__msvc_int128.hpp Outdated Show resolved Hide resolved
stl/inc/__msvc_int128.hpp Outdated Show resolved Hide resolved
stl/inc/__msvc_int128.hpp Outdated Show resolved Hide resolved
stl/inc/__msvc_int128.hpp Outdated Show resolved Hide resolved
stl/inc/__msvc_int128.hpp Outdated Show resolved Hide resolved
with fixes for the failures uncovered. Address review comments.
tests/std/tests/P1522R1_difference_type/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/P1522R1_difference_type/test.cpp Outdated Show resolved Hide resolved
stl/inc/ranges Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej self-assigned this Feb 6, 2022
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@CaseyCarter CaseyCarter merged commit 53d4f93 into microsoft:main Feb 7, 2022
@CaseyCarter CaseyCarter deleted the twenty_for_real branch February 7, 2022 19:36
@CaseyCarter
Copy link
Member Author

CaseyCarter commented Feb 7, 2022

Thank me for pushing to get this done a week ahead of my planned schedule, and extra-special thanks to @StephanTLavavej for the real-time code review to make it possible! ❤️

(Historical note: I bypassed branch policy - as discussed with STL over the weekend - to merge this with only one maintainer review due to time crunch. We did have a few very good contributor reviews to add confidence.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cxx20 C++20 feature defect report Applied retroactively high priority Important!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Determine ABI stability timeline for C++20
6 participants