Skip to content

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

Closed
Mathias R. Jessen (IISResetMe) wants to merge 3 commits into
PowerShell:masterfrom
IISResetMe:enhance-Split-RTL-2
Closed

Enhance -split operator with RightToLeft splitting (#2)#5270
Mathias R. Jessen (IISResetMe) wants to merge 3 commits into
PowerShell:masterfrom
IISResetMe:enhance-Split-RTL-2

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

Mathias R. Jessen (@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

Ilya (@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

Ilya (@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

@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

Mathias R. Jessen (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