Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Enhance -split operator with RightToLeft splitting (#2)#5270

Closed
IISResetMe wants to merge 3 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
IISResetMe:enhance-Split-RTL-2Copy head branch name to clipboard
Closed

Enhance -split operator with RightToLeft splitting (#2)#5270
IISResetMe wants to merge 3 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
IISResetMe:enhance-Split-RTL-2Copy head branch name to clipboard

Conversation

@IISResetMe

Copy link
Copy Markdown
Collaborator

This pull request adds RightToLeft support to the binary -split operator, as requested in #4765

I've added tests for negative integer input for the Max-substrings operand to work with both regex patterns and scriptblocks.

(Originally submitted as #5125, submitted anew due to completely botched rebase of the original branch)

Modified existing test that assumes Max-substrings = 0 when negative. Added tests for scriptblock/predicate-based splitting (regular and right-to-left).
To allow RTL splitting when supplying negative Max-substrings arguments to -split, we set the RightToLeft RegexOption and convert limit to an absolute value
Additionally changed the chunk buffer from StringBuilder to List<char> - we don't use any StringBuilder specific functionality and List<char> allows us to reverse the chunk and avoid Insert(0, item)
The individual chunk lists have been changed to pre-allocate the expected needed capacity to avoid unnecessary resizing of the unuderlying array
@iSazonov

Copy link
Copy Markdown
Collaborator

@IISResetMe I see we could make some optimizations in the code. But your code works well and we can merge. What are your plans? If you want stop and merge I'll ask only some style comments.

@IISResetMe

Copy link
Copy Markdown
Collaborator Author

@iSazonov depends on the extent of your suggested optimizations :-)

I've already included the list allocation changes from the previous PR

@iSazonov

Copy link
Copy Markdown
Collaborator

We can exclude buf StringBuilder using Append(String, In32, Int32) overload.
We can exclude many if (rightToLeft).
We can explude many Reverse().

@IISResetMe

Copy link
Copy Markdown
Collaborator Author

@iSazonov I've already removed the StringBuilder instance in favor of List<char>, but I think I've found a way to simplify the code and avoid most of the if(rightToLeft) branches. I'll fiddle with it and push changes tonight

@SteveL-MSFT SteveL-MSFT mentioned this pull request Jan 9, 2018
5 tasks
@daxian-dbw daxian-dbw self-assigned this Feb 1, 2018
@daxian-dbw daxian-dbw requested review from anmenaga and removed request for BrucePay February 1, 2018 17:55
@lzybkr lzybkr removed their request for review March 14, 2018 16:27
@stale

stale Bot commented Apr 13, 2018

Copy link
Copy Markdown

This PR has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed if no further activity occurs within 10 days.
Thank you for your contributions.
Community members are welcome to grab these works.

@stale stale Bot added the Stale label Apr 13, 2018
@IISResetMe

IISResetMe commented Apr 13, 2018

Copy link
Copy Markdown
Collaborator Author

Closing this for now to avoid too many merge conflicts, will resubmit a new PR when I find the time

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.