Fix Random.Next returns negative numbers #405 #840
Conversation
We need to clear the first bit because rand->Next returns uint32 and the method needs to return int32.
|
Hi @MatthiasJentsch, I'm nanoFramework bot. A human will be reviewing it shortly. 😉 |
| @@ -14,7 +14,7 @@ HRESULT Library_corlib_native_System_Random::Next___I4( CLR_RT_StackFrame& stack | |||
|
|
|||
| NANOCLR_CHECK_HRESULT(GetRandom( stack, rand )); | |||
|
|
|||
| stack.SetResult_I4( rand->Next() ); | |||
| stack.SetResult_I4( rand->Next() & 0x7FFFFFFF ); | |||
There was a problem hiding this comment.
Isn't simply casting a less "expensive" operation as it just gets compiled without requiring any extra operations (ANDing with 0x7FFFFFFF)?
There was a problem hiding this comment.
No. Casting for instance 0xFFFFFFFF which is a valid return value from rand->Next() into an int32 results in the value -1. So I think we need ANDing or modulo operation like % 0x7FFFFFFF
There was a problem hiding this comment.
Personally I am inclined to avoid "magic values" in code. Maybe in the future we could use & Int32.MaxValue which is equivalent to the implemented & 0x7FFFFFFF as it helps the source code to be "self documenting" as to why the specific mask is being applied.
There was a problem hiding this comment.
@doingnz I don't know if we have access to Int32.MaxValue
There was a problem hiding this comment.
I agree with @doingnz .
There is a constant in the CRT lib with that value...
Or maybe a comment explaining why that is being done is good enough.
josesimoes
added
Type: bug
Area: Common libs
Description
We need to clear the first bit because rand->Next returns uint32 and the method needs to return int32.
Motivation and Context
Types of changes
Checklist:
Signed-off-by: Matthias Jentsch info@matthias-jentsch.de