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.

Sign up for GitHub

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

Jump to bottom

Become C++20-compliant in our use of span #6251

Closed
WSLUser opened this issue May 29, 2020 · 3 comments · Fixed by #6908
Closed

Become C++20-compliant in our use of span #6251

WSLUser opened this issue May 29, 2020 · 3 comments · Fixed by #6908
Assignees
@DHowett
Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Help Wanted We encourage anyone to jump in on these. Issue-Task It's a feature request, but it doesn't really need a major design. Priority-2 A description (P2) Product-Conhost For issues in the Console codebase Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone
Terminal Backlog

Comments

@WSLUser
Copy link
Contributor

WSLUser commented May 29, 2020 •
edited
Loading

Description of the new feature/enhancement

After discussing with @DHowett about the issues with updating gsl due to gsl shifting to C++20 std:span behavior and in accordance with below, use of span will be updated to C++20 to minimize changes needed by the update.

A clear and concise description of what the problem is that the new feature would solve.

This will allow us to update gsl.

Proposed technical implementation details (optional)
@WSLUser WSLUser added the Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. label May 29, 2020
@ghost ghost added Needs-Tag-Fix Doesn't match tag requirements Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels May 29, 2020
@DHowett
Copy link
Member

DHowett commented Jun 2, 2020 •
edited
Loading

It's more likely that we'll just move to GSL 3 and become C++20-compliant in our use of span so we can transition off gsl::span with minimal changes later 😄

@WSLUser
Copy link
Contributor Author

WSLUser commented Jun 2, 2020

I'll update this issue to reflect that then as a status tracker. Feel free to edit to add checkboxes if multiple issues need to be created that would tracked in this master issue.

@WSLUser WSLUser changed the title Deps: Adopt span-lite to address std:span issues from gsl Become C++20-compliant in our use of span Jun 2, 2020
@DHowett
Copy link
Member

DHowett commented Jun 2, 2020

Thanks! 😄

@DHowett DHowett added Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Help Wanted We encourage anyone to jump in on these. Issue-Task It's a feature request, but it doesn't really need a major design. Priority-2 A description (P2) and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Jun 2, 2020
@DHowett DHowett added this to the Terminal Backlog milestone Jun 2, 2020
@DHowett DHowett added Product-Conhost For issues in the Console codebase Product-Terminal The new Windows Terminal. labels Jun 2, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Jun 2, 2020
@DHowett DHowett removed the Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. label Jun 2, 2020
@WSLUser WSLUser mentioned this issue Jun 4, 2020
Closed
@DHowett DHowett self-assigned this Jul 14, 2020
DHowett added a commit that referenced this issue Jul 14, 2020
@DHowett
Move to GSL 3.1.0
60d30a3 
GSL 3, the next major version of GSL after the one we're using, replaced
their local implementation of `span` with one that more closely mimics
C++20's span. Unfortunately, that is a breaking change for all of GSL's
consumers.

This commit updates our use of span to comply with the new changes in
GSL 3.

Chief among those breaking changes is:

* `span::at` no longer exists; I replaced many instances of `span::at`
  with `gsl::at(x)`
* `span::size_type` has finally given up on `ptrdiff_t` and become
  `size_t` like all other containers

While I was here, I also made the following mechanical replacements:

* In some of our "early standardized" code, we used std::optional's
  `has_value` and `value` back-to-back. Each `value` incurs an
  additional presence test.
  * Change: `x.value().member` -> `x->member` (`optional::operator->`
    skips the presence test)
* GSL 3 uses `size_t` for `size_type`.
  * Change: `gsl::narrow<size_t>(x.size())` -> `x.size()`
  * Change: `gsl::narrow<ptrdiff_t>(nonSpan.size())` -> `nonSpan.size()`
    during span construction

I also replaced two instances of `x[x.size() - 1]` with `x.back()` and
one instance of a manual array walk (for comparison) with a direct
comparison.

NOTE: Span comparison and `make_span` are not part of the C++20 span
library.

Fixes #6251
@DHowett DHowett mentioned this issue Jul 14, 2020
Merged
@ghost ghost added the In-PR This issue has a related PR label Jul 14, 2020
DHowett added a commit that referenced this issue Jul 14, 2020
@DHowett
Move to GSL 3.1.0
7951030 
GSL 3, the next major version of GSL after the one we're using, replaced
their local implementation of `span` with one that more closely mimics
C++20's span. Unfortunately, that is a breaking change for all of GSL's
consumers.

This commit updates our use of span to comply with the new changes in
GSL 3.

Chief among those breaking changes is:

* `span::at` no longer exists; I replaced many instances of `span::at`
  with `gsl::at(x)`
* `span::size_type` has finally given up on `ptrdiff_t` and become
  `size_t` like all other containers

While I was here, I also made the following mechanical replacements:

* In some of our "early standardized" code, we used std::optional's
  `has_value` and `value` back-to-back. Each `value` incurs an
  additional presence test.
  * Change: `x.value().member` -> `x->member` (`optional::operator->`
    skips the presence test)
* GSL 3 uses `size_t` for `size_type`.
  * Change: `gsl::narrow<size_t>(x.size())` -> `x.size()`
  * Change: `gsl::narrow<ptrdiff_t>(nonSpan.size())` -> `nonSpan.size()`
    during span construction

I also replaced two instances of `x[x.size() - 1]` with `x.back()` and
one instance of a manual array walk (for comparison) with a direct
comparison.

NOTE: Span comparison and `make_span` are not part of the C++20 span
library.

Fixes #6251
@ghost ghost closed this as completed in #6908 Jul 14, 2020
@ghost ghost removed the In-PR This issue has a related PR label Jul 14, 2020
ghost pushed a commit that referenced this issue Jul 14, 2020
@DHowett
Move to GSL 3.1.0 (#6908)
54a7fce 
GSL 3, the next major version of GSL after the one we're using, replaced
their local implementation of `span` with one that more closely mimics
C++20's span. Unfortunately, that is a breaking change for all of GSL's
consumers.

This commit updates our use of span to comply with the new changes in
GSL 3.

Chief among those breaking changes is:

* `span::at` no longer exists; I replaced many instances of `span::at`
  with `gsl::at(x)`
* `span::size_type` has finally given up on `ptrdiff_t` and become
  `size_t` like all other containers

While I was here, I also made the following mechanical replacements:

* In some of our "early standardized" code, we used std::optional's
  `has_value` and `value` back-to-back. Each `value` incurs an
  additional presence test.
  * Change: `x.value().member` -> `x->member` (`optional::operator->`
    skips the presence test)
  * Change: `x.value()` -> `*x` (as above)
* GSL 3 uses `size_t` for `size_type`.
  * Change: `gsl::narrow<size_t>(x.size())` -> `x.size()`
  * Change: `gsl::narrow<ptrdiff_t>(nonSpan.size())` -> `nonSpan.size()`
    during span construction

I also replaced two instances of `x[x.size() - 1]` with `x.back()` and
one instance of a manual array walk (for comparison) with a direct
comparison.

NOTE: Span comparison and `make_span` are not part of the C++20 span
library.

Fixes #6251
@ghost ghost added the Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. label Jul 14, 2020
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@DHowett DHowett

Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Help Wanted We encourage anyone to jump in on these. Issue-Task It's a feature request, but it doesn't really need a major design. Priority-2 A description (P2) Product-Conhost For issues in the Console codebase Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Milestone
Terminal Backlog
Development

Successfully merging a pull request may close this issue.

Move to GSL 3.1.0 microsoft/terminal
2 participants
@DHowett @WSLUser