Clean up Get-Random cmdlet #9133
Conversation
|
|
||
| #endregion | ||
|
|
||
| #region Cmdlet processing methods | ||
|
|
||
| private double GetRandomDouble(double min, double max) | ||
| { | ||
| double randomNumber; | ||
| double result; |
There was a problem hiding this comment.
The original name makes more sense. Please revert.
| } | ||
|
|
||
| return randomNumber; | ||
| Debug.Assert(min <= result, "lower bound <= random number"); | ||
| Debug.Assert(result < max, "random number < upper bound"); |
There was a problem hiding this comment.
Why first condition is <= but second one <?
There was a problem hiding this comment.
GetRandomDouble returns a random number between minValue (included) and maxValue (excluded). Hence the presence of the do{}while(randomNumber >= maxValue) loop
| randomUint64 = BitConverter.ToUInt64(buffer, 0); | ||
| // Get the last 'bitsToRepresentDiff' number of randon bits | ||
| randomUint64 &= mask; | ||
| } while (uint64Diff <= randomUint64); | ||
|
|
||
| double result = min * 1.0 + randomUint64 * 1.0; | ||
|
|
||
| Debug.Assert(min <= result, "lower bound <= random number"); | ||
| Debug.Assert(result < max, "random number < upper bound"); |
There was a problem hiding this comment.
For this case I'm not too sure.
At line 347 it will try to enforce maxValue as an excluding upper bound due to the Next(min, max) method but in this scenario I'm not sure if it will enforce maxValue as an exclusive upper bound.
In doubt I think it's best to at least stick with previous behavior in which the Assert came after the function call thus trying to enforce maxValue as an excluded upper bound in all cases. It's safer to have a script fail than to have it run with an unexpected value.
@PaulHigin ?
There was a problem hiding this comment.
I'd expect the same behavior (included or excluded) for both low and high boundaries.
Although we can guarantee this only for integers.
There was a problem hiding this comment.
In that case :
- do you consider this resolved ?
- do you want different groups of debug.assert statements for each of the two scenarios I listed where each assert correctly follows what the generator should return as a value ?
- do you want a single debug.assert group which would be this one but with randomValue <= upper bound ?
- do you want @PaulHigin 's take on this ?
😄
There was a problem hiding this comment.
@pougetat Please remove all controversial changes from this PR so that we can move forward
There was a problem hiding this comment.
ok ill put the debug.assert statements back where they were.
| /// <returns></returns> | ||
| public int Next(int minValue, int maxValue) | ||
| public int Next(int min, int max) |
There was a problem hiding this comment.
The original names makes more sense. Please revert.
test/powershell/Modules/Microsoft.PowerShell.Utility/Get-Random.Tests.ps1
Outdated
Show resolved
Hide resolved
test/powershell/Modules/Microsoft.PowerShell.Utility/Get-Random.Tests.ps1
Outdated
Show resolved
Hide resolved
…m.Tests.ps1 Co-Authored-By: pougetat <thomas.pouget-abadie@ensimag.grenoble-inp.fr>
…m.Tests.ps1 Co-Authored-By: pougetat <thomas.pouget-abadie@ensimag.grenoble-inp.fr>
src/Microsoft.PowerShell.Commands.Utility/commands/utility/GetRandomCommand.cs
Show resolved
Hide resolved
…RandomCommand.cs Co-Authored-By: pougetat <thomas.pouget-abadie@ensimag.grenoble-inp.fr>
|
@iSazonov Made the last modif :) |
PR Summary
This PR cleans up the coding style in the class implementing Get-Random to make it consistent with the rest of the codebase
PR Context
This PR is linked to the following one and should be merged beforehand : #9111.
Once it is merged I will checkout 9111 and rebase master.
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile 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