Replace gsl::at with a new til::at(span) for pre-checked bounds #6925
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We were using std::basic_string_view as a stand-in for std::span so that we could change over all at once when C++20 dropped with full span support. That day's not here yet, but as of 54a7fce we're using GSL 3, whose span is C++20-compliant. This commit replaces every instance of basic_string_view that was not referring to an actual string with a span of the appropriate type. I moved the `const` qualifier into span's `T` because while `basic_string_view.at()` returns `const T&`, `span.at()` returns `T&` (without the const). I wanted to maintain the invariant that members of the span were immutable. * Mechanical Changes * `sv.at(x)` -> `gsl::at(sp, x)` I had to replace a `std::basic_string<>` with a `std::vector>` in ConImeInfo, and I chose to replace a manual array walk in ScreenInfoUiaProviderBase with a ranged-for. Please review those specifically. This will almost certainly cause a code size regression in Windows because I'm blowing out all the PGO counts. Whoops. Related: #3956, #975.
The recent changes to use gsl::span everywhere added a few bounds checks along codepaths where we were already checking bounds. Some of them may be non-obvious to the optimizer, so we can now use til::at to help them along. To achieve this, I've added a new overload of til::at that takes a span and directly accesses its backing buffer.
zadjii-msft
approved these changes
Jul 15, 2020
| }; | ||
|
|
||
| template<class T> | ||
| struct is_span : public is_span_oracle<std::remove_cv_t<T>> |
There was a problem hiding this comment.
Alright well I'll file this PR under the list of "C++ is a made-up witchcraft language"
zadjii-msft
added
the
Area-CodeHealth
miniksa
approved these changes
Jul 15, 2020
|
depends on #6921 |
|
This PR is absolutely horked thanks to its base branch being deleted. Recovering. |
DHowett
added
the
AutoMerge
|
Hello @DHowett! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
DHowett
removed
the
AutoMerge
DHowett
added a commit
that referenced
this pull request
Aug 5, 2020
- Move to GSL 3.1.0 (GH-6908) - Replace the color table init code with two const arrays (GH-6913) - Replace basic_string_view<T> with span<const T> (GH-6921) - Replace gsl::at with a new til::at(span) for pre-checked bounds (GH-6925) Related work items: MSFT:27866336
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The recent changes to use gsl::span everywhere added a few bounds checks
along codepaths where we were already checking bounds. Some of them may
be non-obvious to the optimizer, so we can now use til::at to help them
along.
To accomplish this, I've added a new overload of til::at that takes a
span and directly accesses its backing buffer.