Use span-based overloads by iSazonov · Pull Request #11884 · PowerShell/PowerShell · GitHub

PR Summary

First commit - Use span-based overloads to reduce allocations.
Second commit - Use String.Contains() for readability.

PR Context

PR Checklist

PR has a meaningful title
- Use the present tense and imperative mood when describing your changes
Summarized changes
Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
This PR is ready to merge and is not Work in Progress.
- If the PR is work in progress, please add the prefix WIP: or [ WIP ] to the beginning of the title (the WIP bot will keep its status check at Pending while the prefix is present) and remove the prefix when the PR is ready.
Breaking changes
- None
- OR
- Experimental feature(s) needed
  - Experimental feature name(s):
User-facing changes
- Not Applicable
- OR
- Documentation needed
  - Issue filed:
Testing - New and feature
- N/A or can only be tested interactively
- OR
- Make sure you've added a new test if existing tests do not effectively test the code changed
Tooling
- I have considered the user experience from a tooling perspective and don't believe tooling will be impacted.
- OR
- I have considered the user experience from a tooling perspective and enumerated concerns in the summary. This may include:
  - Impact on PowerShell Editor Services which is used in the PowerShell extension for VSCode (which runs in a different PS Host).
  - Impact on Completions (both in the console and in editors) - one of PowerShell's most powerful features.
  - Impact on PSScriptAnalyzer (which provides linting & formatting in the editor extensions).
  - Impact on EditorSyntax (which provides syntax highlighting with in VSCode, GitHub, and many other editors).

ToString() here results in an allocation, can we just update that method to accept a readonlyspan?

No, it is used then in class ctor-s.

ReadOnlySpan<char> typically is an implicit cast from string, isn't it?

Rain Sallow (/u/ta11ow) (@vexx32) No, only string -> ReadOnlySpan

Right, so if the method parameter is currently using string we should be able to change it to ReadOnlySpan<char> and have the implicit cast handle it, shouldn't we? Or am I missing some context here? 🙂

I missing some context here

CreateParameterWithArgument() passes the string to a constructor so it makes no sense to convert to span here.

Added new commit.

LGTM

Please use multiple statements for better readability, like

var filterSpan = filter.AsSpan(0, filter.Length - 2);
return commandName.AsSpan().Contains(filterSpan, StringComparison.OrdinalIgnoreCase);

Fixed.

same here :)

Fixed.

🎉v7.1.0-preview.1 has been released which incorporates this pull request.:tada:

Handy links:

Release Notes

Ilya (iSazonov) added the CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log label Feb 19, 2020

Ilya (iSazonov) added this to the 7.1.0-preview.1 milestone Feb 19, 2020

Ilya (iSazonov) requested a review from Steve Lee (SteveL-MSFT) February 19, 2020 11:07

Ilya (iSazonov) requested review from Aditya Patwardhan (adityapatwardhan), Andrew (anmenaga) and Dongbo Wang (daxian-dbw) as code owners February 19, 2020 11:07

ghost assigned Aditya Patwardhan (adityapatwardhan) Feb 19, 2020

PoshChan-Bot (PoshChan) reviewed Feb 19, 2020

View reviewed changes

Comment thread src/Microsoft.Management.UI.Internal/ShowCommand/ViewModel/ModuleViewModel.cs Outdated

Steve Lee (SteveL-MSFT) reviewed Feb 19, 2020

View reviewed changes

PoshChan-Bot (PoshChan) reviewed Feb 20, 2020

View reviewed changes

Comment thread src/Microsoft.Management.UI.Internal/ShowCommand/ViewModel/CommandViewModel.cs Outdated

PoshChan-Bot (PoshChan) reviewed Feb 20, 2020

View reviewed changes

Comment thread src/Microsoft.Management.UI.Internal/ShowCommand/ViewModel/CommandViewModel.cs Outdated

PoshChan-Bot (PoshChan) reviewed Feb 20, 2020

View reviewed changes

Comment thread src/Microsoft.Management.UI.Internal/ShowCommand/ViewModel/CommandViewModel.cs Outdated

Ilya (iSazonov) force-pushed the cleanup-use-span-overloads branch from 2627a51 to 372c510 Compare February 20, 2020 09:16

Ilya (iSazonov) force-pushed the cleanup-use-span-overloads branch from 372c510 to 3e3fad1 Compare March 9, 2020 17:31

Steve Lee (SteveL-MSFT) approved these changes Mar 9, 2020

View reviewed changes

Dongbo Wang (daxian-dbw) reviewed Mar 13, 2020

View reviewed changes

Ilya (iSazonov) added 3 commits March 13, 2020 11:56

Use Span-based overloads

9cefaa9

Use String.Contains()

3296ac0

Address feedback

5d7aeed

Ilya (iSazonov) force-pushed the cleanup-use-span-overloads branch from 3e3fad1 to 5d7aeed Compare March 13, 2020 06:57

Ilya (iSazonov) assigned Ilya (iSazonov) and unassigned Aditya Patwardhan (adityapatwardhan) Mar 18, 2020

Ilya (iSazonov) merged commit b5d4739 into PowerShell:master Mar 18, 2020

Ilya (iSazonov) deleted the cleanup-use-span-overloads branch March 18, 2020 13:39

Ilya (iSazonov) mentioned this pull request Oct 29, 2020

Fix typo in MapSecurityZone #13921

Merged

14 tasks

Ilya (iSazonov) mentioned this pull request Jan 3, 2023

Fix regression in RemoveNulls #18881

Merged

22 tasks

Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj/PowerShell that referenced this pull request Aug 6, 2026

Use span-based overloads (PowerShell#11884 )

df513b0

Use span-based overloads by iSazonov · Pull Request #11884 · PowerShell/PowerShell · GitHub

Uh oh!

Use span-based overloads - #11884

Use span-based overloads#11884Ilya (iSazonov) merged 3 commits intoPowerShell:masterPowerShell/PowerShell:masterfrom iSazonov:cleanup-use-span-overloadsiSazonov/PowerShell:cleanup-use-span-overloadsCopy head branch name to clipboard

Conversation

Ilya (iSazonov) commented Feb 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Context

PR Checklist

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) Feb 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rain Sallow (/u/ta11ow) (vexx32) Feb 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) commented Feb 20, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Steve Lee (SteveL-MSFT) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Deleted user (ghost) commented Mar 26, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Use span-based overloads#11884
Ilya (iSazonov) merged 3 commits into
PowerShell:masterfrom
iSazonov:cleanup-use-span-overloads

Ilya (iSazonov) commented Feb 19, 2020 •
edited
Loading

Ilya (iSazonov) Feb 20, 2020 •
edited
Loading

Rain Sallow (/u/ta11ow) (vexx32) Feb 21, 2020 •
edited
Loading