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

<numeric>: check for gcd / lcm overflows #4776

Merged
merged 14 commits into from
Jul 11, 2024

Conversation

AlexGuteniev
Copy link
Contributor

@AlexGuteniev AlexGuteniev commented Jul 1, 2024

In compile time or in debug.

Fixes #2300

In compile time or in debug.
Applies to C++20 and later mode.
@AlexGuteniev AlexGuteniev requested a review from a team as a code owner July 1, 2024 12:12
stl/inc/numeric Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label Jul 1, 2024
@StephanTLavavej StephanTLavavej self-assigned this Jul 1, 2024
stl/inc/numeric Outdated Show resolved Hide resolved
stl/inc/numeric Outdated Show resolved Hide resolved
stl/inc/numeric Outdated Show resolved Hide resolved
stl/inc/numeric Outdated Show resolved Hide resolved
stl/inc/numeric Outdated Show resolved Hide resolved
stl/inc/xutility Show resolved Hide resolved
@StephanTLavavej StephanTLavavej removed their assignment Jul 9, 2024
@StephanTLavavej StephanTLavavej self-assigned this Jul 10, 2024
@StephanTLavavej
Copy link
Member

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

numeric: Add a `static_cast` when dividing `_Common_unsigned` by `_Common_unsigned`,
due to the usual arithmetic conversions which will emit sign conversion warnings for tiny types.

utility:
* Move `_Is_standard_integer` down to the `_HAS_CXX20` region. (It's also used by `<mdspan>`.)
* Relax the internal `_Cmp_equal`, `_Cmp_less`, and `_In_range` to accept nonbool integrals.
  + Because they're internal, we can `_STL_INTERNAL_STATIC_ASSERT`.
  + Comment that this "allows character types".
* Enforce `_Is_standard_integer` at the user-visible layer.

P0295R0_gcd_lcm: Add test coverage, because gcd/lcm are required to accept nonbool integrals.
(An MSVC-internal test found this by using `char`.)
@StephanTLavavej
Copy link
Member

I pushed a commit:

Fix gcd/lcm to work with all nonbool integrals.

numeric: Add a static_cast when dividing _Common_unsigned by _Common_unsigned, due to the usual arithmetic conversions which will emit sign conversion warnings for tiny types.

utility:

  • Move _Is_standard_integer down to the _HAS_CXX20 region. (It's also used by <mdspan>.)
  • Relax the internal _Cmp_equal, _Cmp_less, and _In_range to accept nonbool integrals.
    • Because they're internal, we can _STL_INTERNAL_STATIC_ASSERT.
    • Comment that this "allows character types".
  • Enforce _Is_standard_integer at the user-visible layer.

P0295R0_gcd_lcm: Add test coverage, because gcd/lcm are required to accept nonbool integrals. (An MSVC-internal test found this by using char.)

@@ -883,38 +876,56 @@ _NODISCARD constexpr bool _In_range(const _Ty _Value) noexcept {
}

#if _HAS_CXX20
template <class _Ty>
constexpr bool _Is_standard_integer = _Is_any_of_v<remove_cv_t<_Ty>, signed char, short, int, long, long long,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could be useful to comment that this can be used to test for standard and extended integer types, because there are no extended integer types (probably in a subsequent change)

@StephanTLavavej StephanTLavavej merged commit 39dd503 into microsoft:main Jul 11, 2024
39 checks passed
@StephanTLavavej
Copy link
Member

Thanks for implementing these checks! My internal branch was dev/stl/least-common-meowtiple 😹 🧮 📈

@AlexGuteniev AlexGuteniev deleted the overflow branch July 12, 2024 04:42
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
Archived in project
Development

Successfully merging this pull request may close these issues.

<numeric>: gcd and lcm do not check for negative overflow in compile time
3 participants