-
Notifications
You must be signed in to change notification settings - Fork 7.1k
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.
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
Refactor Shuffle in Get-Random to save a full iteration of the objects. #8969
Refactor Shuffle in Get-Random to save a full iteration of the objects. #8969
Conversation
src/Microsoft.PowerShell.Commands.Utility/commands/utility/GetRandomCommand.cs
Outdated
Show resolved
Hide resolved
object tmp = _chosenListItems[i]; | ||
_chosenListItems[i] = _chosenListItems[j]; | ||
_chosenListItems[j] = tmp; | ||
_chosenListItems[j] = _chosenListItems[i]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this cause the value in i
to be duplicated? Shouldn't we swap like we did before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand now, the assignment effectively removes the output value from the list for the next iteration. The comment should be updated to make this more clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do it soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update code comment to make assignment rather than swap change more clear.
object tmp = _chosenListItems[i]; | ||
_chosenListItems[i] = _chosenListItems[j]; | ||
_chosenListItems[j] = tmp; | ||
_chosenListItems[j] = _chosenListItems[i]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand now, the assignment effectively removes the output value from the list for the next iteration. The comment should be updated to make this more clear.
How do I trigger the CI Build without a dummy commit? The build broke due to an unrelated change |
Just ask and a maintainer can do it for you. Otherwise it requires a dummy commit. In this scenario, asking was better. |
But looking at the history, it looks like GitHub is not remerging your PR. You might need to rebase your PR to reliably get a clean Windows CI. |
I can do that for you if you don't know how, but the failures I saw are intermittent. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@st0le Please fix the style issues and we'll merge.
PR Summary
Minor refactoring of the Fisher Yates shuffle to save an additional iteration.
PR Context
Micro-optimization to the shuffle function.
PR Checklist
.h
,.cpp
,.cs
,.ps1
and.psm1
files have the correct copyright headerWIP:
or[ WIP ]
to the beginning of the title (theWIP
bot will keep its status check atPending
while the prefix is present) and remove the prefix when the PR is ready.[feature]
to your commit messages if the change is significant or affects feature tests