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

Refactor <limits> usage #4634

Merged
merged 32 commits into from May 20, 2024
Merged

Conversation

AlexGuteniev
Copy link
Contributor

Based on #4627 fixup that extracts _In_range.
This also makes some place directly using _In_range, but mostly _Max_limit is used.

Based on microsoft#4627 fixup that extracts `_In_range`
This also makes some place dirctly using `_In_range`,
but mostly `_Max_limit` is used
@AlexGuteniev AlexGuteniev requested a review from a team as a code owner April 27, 2024 11:51
@AlexGuteniev
Copy link
Contributor Author

libc++ tests failures are reported upstream as LLVM-90345

@AlexGuteniev

This comment was marked as resolved.

@StephanTLavavej StephanTLavavej added the throughput Must compile faster label Apr 27, 2024
@StephanTLavavej StephanTLavavej self-assigned this Apr 27, 2024
stl/inc/xmemory Outdated Show resolved Hide resolved
stl/inc/xlocnum Outdated Show resolved Hide resolved
@AlexGuteniev
Copy link
Contributor Author

I'm not sure if this change is correct though, as maybe the parts I've changed need to support custom integer-like types:

  • <algorithm> and <execution> may need to support custom iterators with custom integer-like type as difference_type
  • containers may need to support custom allocators with custom integer-like types as size_type / difference_type

These custom integer-like types may provide a specialization of numeric_limits, but not _Max_limit.

@frederick-vs-ja
Copy link
Contributor

  • <algorithm> and <execution> may need to support custom iterators with custom integer-like type as difference_type

Integer-like types are not supposed to be customizable. They're either integer types or integer-class types provided by the implementation. The requirements in [iterator.concept.winc] don't seem implementable if a user-defined type is allowed to be an integer-class type.

In MSVC STL, we only need to specially handle _Signed128 and _Unsigned128 (and the latter shoudn't be difference_type).

  • containers may need to support custom allocators with custom integer-like types as size_type / difference_type

Nope. The standard clearly says that their size_type and difference_type are integer types ([allocator.requirements.general], [container.reqmts]).

ldionne pushed a commit to llvm/llvm-project that referenced this pull request Apr 30, 2024
* This was unnecessarily asserting *strictly* less than `ULONG_MAX`, not that it mattered.
* We can/should use Standard `in_range` because `<semaphore>` is C++20.
* We control `_Ten_days`, so this should be an `_STL_INTERNAL_STATIC_ASSERT`.
* As a scalar, `_Ten_days` doesn't need to be `static constexpr`, it can/should be plain `constexpr`.
We know `is_integral_v<_Elem>` and `is_signed_v<_Elem>` so this is definitely ok.
This is `_Integer_like _Int`, which is ok because we handle `_Signed128` and `_Unsigned128`.

The first case is simple: `_Int` is known to be unsigned, and we want the maximum.

The second case requires a bit of thought. `_Int` is known to be signed, and `_UInt` is its unsigned counterpart. This code wants the maximum `_Int`, stored in `_UInt` type. The new function makes this clearer than the old math.
The following headers are no longer including `<limits>`:

* C++14 `<xmemory>`
* C++20 `<format>`
* C++23 `<mdspan>`

The installed base of C++20/23 code is far smaller, and far more modern. We don't need escape hatches for them.

However, we should have an escape hatch for the `<xmemory>` change, as I expect that this will otherwise be too disruptive.
stl/inc/__msvc_int128.hpp Show resolved Hide resolved
stl/inc/algorithm Outdated Show resolved Hide resolved
stl/inc/vector Outdated Show resolved Hide resolved
stl/inc/mdspan Outdated Show resolved Hide resolved
stl/inc/xlocnum Outdated Show resolved Hide resolved
stl/inc/format Outdated Show resolved Hide resolved
@StephanTLavavej
Copy link
Member

Thanks! I pushed several commits:

  • Add _Min_limit for _Unsigned128 and _Signed128.
  • Add _STD qualification in <mdspan>.
  • Restore inclusion: <xlocnum> needs <climits>.
  • _In_range: Use it more often; we control all types.
  • in_range: Use it in <semaphore> with bonus cleanups.
    • This was unnecessarily asserting strictly less than ULONG_MAX, not that it mattered.
    • We can/should use Standard in_range because <semaphore> is C++20.
    • We control _Ten_days, so this should be an _STL_INTERNAL_STATIC_ASSERT.
    • As a scalar, _Ten_days doesn't need to be static constexpr, it can/should be plain constexpr.
  • in_range: Use it in <format> and chronat.
  • _Meow_limit: Use it in _Could_compare_equal_to_value_type.
    • We know is_integral_v<_Elem> and is_signed_v<_Elem> so this is definitely ok.
  • _Max_limit: Use it in _Mul_overflow.
    • This is _Integer_like _Int, which is ok because we handle _Signed128 and _Unsigned128.
    • The first case is simple: _Int is known to be unsigned, and we want the maximum.
    • The second case requires a bit of thought. _Int is known to be signed, and _UInt is its unsigned counterpart. This code wants the maximum _Int, stored in _UInt type. The new function makes this clearer than the old math.
  • Be daring, part 1: <algorithm> and <vector> are now unlimited.
  • Be daring, part 2: FAIL the other tests fixed by LLVM-90345.
  • Be daring, part 3: Add an escape hatch.
    • The following headers are no longer including <limits>:
      • C++14 <xmemory>
      • C++20 <format>
      • C++23 <mdspan>
    • The installed base of C++20/23 code is far smaller, and far more modern. We don't need escape hatches for them.
    • However, we should have an escape hatch for the <xmemory> change, as I expect that this will otherwise be too disruptive.

@StephanTLavavej StephanTLavavej removed their assignment May 17, 2024
@AlexGuteniev
Copy link
Contributor Author

  • _Meow_limit: Use it in _Could_compare_equal_to_value_type.

I thought that was #4646

@AlexGuteniev
Copy link
Contributor Author

My rationale for separate #4646 was a speculation that this one might be pulled out; otherwise #4646 belongs here.

@StephanTLavavej
Copy link
Member

Sorry, I've been context-switched too much for the last couple of weeks and I lost track of all of the in-flight changes.

Let's consolidate into this one. I'll validate and push changes.

@StephanTLavavej StephanTLavavej self-assigned this May 17, 2024
@StephanTLavavej
Copy link
Member

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

…de `<cwchar>` in `<limits>`".

Co-authored-by: A. Jiang <[email protected]>
@StephanTLavavej
Copy link
Member

Removing <limits> from <xmemory> broke 9 files in the compiler back-end's sources, and another 9 files in the front-end's tests. I fixed all of them to make the next attempt easier, but for now I'm undoing that change because the RWC impact will surely be too much for me to handle right now. I added a comment as a reminder why we're keeping this unnecessary include. Future maintainers (maybe me again) can come back and deal with this later.

Because it's closely related, I partially resurrected @frederick-vs-ja's #3631, taking the changes to make <limits> no longer rely on <cwchar>, but commenting why we're keeping that inclusion.

(There were several breaks assuming each step of the Lots Of Stuff => <xmemory> => <limits> => <cwchar> => <cstdio> chain.)

@StephanTLavavej StephanTLavavej changed the title <limits>: avoid including to improve throughput Refactor <limits> usage May 18, 2024
@StephanTLavavej StephanTLavavej added enhancement Something can be improved and removed throughput Must compile faster labels May 18, 2024
@StephanTLavavej StephanTLavavej merged commit 697653d into microsoft:main May 20, 2024
39 checks passed
@StephanTLavavej
Copy link
Member

Thanks for cleaning this up! The in_range improvements are great, and we're getting closer to making this <limits> breaking change someday. 📈 💥 😹

@AlexGuteniev AlexGuteniev deleted the no_limits branch May 21, 2024 04:29
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.

None yet

3 participants