[no squash] Unittest and some documentation things #14238
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sfan5
added
Maintenance
sfan5
force-pushed
the
psr
branch
4 times, most recently
from
152c9d9
to
aabf974
Compare
sfan5
force-pushed
the
psr
branch
5 times, most recently
from
f28f7a6
to
eb5ecab
Compare
grorp
reviewed
Jan 12, 2024
sfan5
force-pushed
the
psr
branch
3 times, most recently
from
607d892
to
1829160
Compare
appgurueu
reviewed
Jan 13, 2024
appgurueu
reviewed
Jan 13, 2024
| { | ||
| m_next = seed; | ||
| } | ||
|
|
||
| inline int next() | ||
| inline u32 next() | ||
| { | ||
| m_next = m_next * 1103515245 + 12345; |
There was a problem hiding this comment.
Out of scope for this PR to fix, but isn't there some integer overflow UB here?
There was a problem hiding this comment.
I wondered about that too and signed overflow is indeed UB.
Converting it to unsigned (while keeping compatibility with the negative seeds) would be a bother...
appgurueu
reviewed
Jan 13, 2024
| @@ -70,14 +70,14 @@ class PseudoRandom { | |||
| PcgRandom, we cannot modify this RNG's range as it would change the | |||
| output of this RNG for reverse compatibility. | |||
| */ | |||
| if ((u32)(max - min) > (RANDOM_RANGE + 1) / 10) | |||
| if ((u32)(max - min) > (RANDOM_RANGE + 1) / 5) | |||
There was a problem hiding this comment.
Why does the 10 become 5?
appgurueu
reviewed
Jan 13, 2024
| @@ -8057,7 +8057,9 @@ A 32-bit pseudorandom number generator. | |||
| Uses PCG32, an algorithm of the permuted congruential generator family, | |||
| offering very strong randomness. | |||
|
|
|||
| It can be created via `PcgRandom(seed)` or `PcgRandom(seed, sequence)`. | |||
| * constructor `PcgRandom(seed, [seq])` | |||
| * `seed`: 64-bit unsigned seed | |||
There was a problem hiding this comment.
Not this PR's job to fix, but this is fishy since on the Lua side of things, we can only represent integers up to 2^53 accurately.
appgurueu
reviewed
Jan 13, 2024
appgurueu
reviewed
Jan 13, 2024
The path being tested for is in the default search path for games, so it would still pass if the env var was not working.
Also extends the allowed range on the C++ side. This has no side-effects.
I think it's better suited here than in the wiki.
appgurueu
approved these changes
Jan 13, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
notable thing: you can now properly run unit tests from an installed MT build because it no longer tries to read from
/home/you/where/ever/the/source/was(when it isn't anymore).it will still skip some if you don't have devtest however.