8000 Replace StringCollection with generic List by xtqqczze · Pull Request #13431 · PowerShell/PowerShell · GitHub
[go: up one dir, main page]
Skip to content

Replace StringCollection with generic List #13431

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
Closed
wants to merge 6 commits into from
Closed

Replace StringCollection with generic List #13431

wants to merge 6 commits into from

Conversation

xtqqczze
Copy link
Contributor
@xtqqczze xtqqczze commented Aug 14, 2020 •
edited
Loading

PR Summary

  • Replace StringCollection with List<string>.
  • Replace StringCollection with string[] when length is known.
  • Rename variables for clarity.
  • Refactor GenerateMultiLineRowField as static method.
  • Cache loop invariant integers.

Changes not made in the following:
src\System.Management.Automation\engine\LanguagePrimitives.cs: complex call hierarchy
src\Microsoft.PowerShell.Commands.Diagnostics\CounterSet.cs: parameter in public API

PR Context

Performance using the generic List<string> is much better than using the non-generic StringCollection.

See #13425 (comment) for a benchmark.

PR Checklist

@ghost ghost assigned daxian-dbw Aug 14, 2020
@xtqqczze xtqqczze mentioned this pull request Aug 14, 2020
Closed
8 tasks
@xtqqczze

This comment has been minimized.

Sign in to view
@iSazonov iSazonov added the CL-Performance Indicates that a PR should be marked as a performance improvement in the Change Log label Aug 14, 2020
@xtqqczze
Copy link
Contributor Author

@rjmholt Could we make changes in LanguagePrimitives.cs?

@ghost ghost added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept labels Aug 17, 2020
@xtqqczze xtqqczze marked this pull request as ready for review August 22, 2020 15:52
@xtqqczze
Copy link
Contributor Author

Existing formatting was left unchanged, hence Codacy issues.

@xtqqczze
Copy link
Contributor Author

@iSazonov can you approve?

@ghost ghost added the Review - Needed The PR is being reviewed label Sep 5, 2020
@ghost
Copy link
ghost commented Sep 5, 2020

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

@xtqqczze xtqqczze requested a review from rjmholt October 22, 2020 14:25
@xtqqczze xtqqczze force-pushed the replace-StringCollection branch from 161da42 to a7a3160 Compare October 24, 2020 17:16
@xtqqczze
Copy link
Contributor Author

rebased to remove merge commit

@iSazonov
Copy link
Collaborator
iSazonov commented Nov 5, 2020

@xtqqczze Please resolve merge conflicts.

@ghost ghost removed the Review - Needed The PR is being reviewed label Nov 5, 2020
@iSazonov
Copy link
Collaborator
iSazonov commented Nov 6, 2020

@xtqqczze I think it is better to split the PR to some small PRs by cmdlet/folder to get more fast progress.

@ghost ghost added the Review - Needed The PR is being reviewed label Nov 13, 2020
@ghost
Copy link
ghost commented Nov 13, 2020

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

@xtqqczze xtqqczze marked this pull request as draft November 13, 2020 21:53
@ghost ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Nov 17, 2020
@iSazonov
Copy link
Collaborator
iSazonov commented Nov 21, 2020 •
edited
Loading

Please look CI fails.

  • Replace StringCollection with List<string>.
  • Replace StringCollection with string[] when length is known.
  • Rename variables for clarity.
  • Refactor GenerateMultiLineRowField as static method.
  • Cache loop invariant integers.

and maybe split the PR by kind of changes since there is a risk of regression.

@ghost ghost removed the Review - Needed The PR is being reviewed label Nov 21, 2020
@ghost ghost added the Stale label Dec 6, 2020
@ghost
Copy link
ghost commented Dec 6, 2020

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 15 days. It will be closed if no further activity occurs within 10 days of this comment.

@ghost ghost closed this Dec 16, 2020
@xtqqczze xtqqczze mentioned this pull request Jul 27, 2021
Merged
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iSazonov iSazonov iSazonov approved these changes

@adityapatwardhan adityapatwardhan Awaiting requested review from adityapatwardhan

@daxian-dbw daxian-dbw Awaiting requested review from daxian-dbw

@rjmholt rjmholt Awaiting requested review from rjmholt

Assignees

@daxian-dbw daxian-dbw

Labels
CL-Performance Indicates that a PR should be marked as a performance improvement in the Change Log Stale Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@xtqqczze @iSazonov @rjmholt @daxian-dbw
0