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

<__msvc_int128.hpp>: Backport 128-bit integer-class types to C++14 mode #3036

Merged
merged 26 commits into from
Aug 31, 2022

Conversation

frederick-vs-ja
Copy link
Contributor

Thanks to @MattStephanson for having done the most works in #3012. As mentioned in #3012, having 128-bit integer-like types in all modes is useful for random number generation.

This PR also provides a library solution for DevCom-879048 with literal suffixes (__i128 and __u128) support.

frederick-vs-ja and others added 2 commits August 16, 2022 14:27
@frederick-vs-ja frederick-vs-ja requested a review from a team as a code owner August 16, 2022 06:43
@frederick-vs-ja
Copy link
Contributor Author

Sometimes some (possibly unrelated) tests run forever. Is this triggering some compiler bug?

Copy link
Contributor

@strega-nil-ms strega-nil-ms left a comment

Choose a reason for hiding this comment

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

saw some stuff, but I need to go pass out again (think I'm getting sick)

stl/inc/__msvc_int128.hpp Outdated Show resolved Hide resolved
stl/inc/__msvc_int128.hpp Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label Aug 16, 2022
@StephanTLavavej StephanTLavavej added this to Initial Review in Code Reviews via automation Aug 16, 2022
Co-authored-by: nicole mazzuca <83086508+strega-nil-ms@users.noreply.github.com>
Copy link
Contributor

@strega-nil-ms strega-nil-ms left a comment

Choose a reason for hiding this comment

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

Other than that, looks really good! thanks!

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
Code Reviews automation moved this from Initial Review to Work In Progress Aug 24, 2022
@strega-nil-ms
Copy link
Contributor

Kiki (from Kiki's Delivery Service) looking happy and saying 'Thank You!'

@StephanTLavavej StephanTLavavej moved this from Work In Progress to Final Review in Code Reviews Aug 26, 2022
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

Thanks, this looks good! I pushed various minor changes:

  • Consistent preprocessor comments.
  • Always use braces for control flow.
  • Already constrained for is_integral_v.
  • Don't need to qualify numeric_limits.
  • Rename to _Int128_detail.
  • Add newline between non-chained if-statements.
  • Unnecessary parentheses.
  • _NODISCARD UDLs.
  • _NODISCARD helper functions.
  • Test capital 0B.
  • Test capital hexits.

@StephanTLavavej StephanTLavavej moved this from Final Review to Ready To Merge in Code Reviews Aug 30, 2022
@StephanTLavavej StephanTLavavej self-assigned this Aug 31, 2022
@StephanTLavavej
Copy link
Member

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

@StephanTLavavej
Copy link
Member

I had to push two more commits to fix internal test failures:

  • EDG found two missed occurrences of _ZERO_OR_NO_INIT (I'm not sure why this was only found internally, but yay)
  • /clr:pure, the betrayer of hope, has notorious trouble with variable templates. I've converted the logic into struct specializations dispatching to a static member function.

@StephanTLavavej
Copy link
Member

I had to push two more commits to fix internal test failures for /clr and /clr:pure on x64.

  • /clr:pure lacks virtually all intrinsics, so as usual we must avoid using them. Additionally, to avoid headaches, I am also disabling intrinsics for CUDA and the Intel compiler.
  • There is a silent bad codegen bug for plain /clr when asking the result of -6 multiplied by -2. I've added a workaround but haven't reduced/reported it.

@StephanTLavavej StephanTLavavej merged commit f441292 into microsoft:main Aug 31, 2022
Code Reviews automation moved this from Ready To Merge to Done Aug 31, 2022
@StephanTLavavej
Copy link
Member

Thanks for bringing this Future Technology to the past! 🤖 ⌚ 🛰️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Something can be improved
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants