Fix: [Script] ScriptBase::Rand() return value was between -MIN(int32) and MAX(int32) #10443
Conversation
|
|
||
| #include "../../safeguards.h" | ||
|
|
||
| /* static */ uint32 ScriptBase::Rand() | ||
| /* static */ SQInteger ScriptBase::Rand() |
There was a problem hiding this comment.
Isn't it better to use int64 than SQInteger? I always thought SQInteger was specifically used outside the API.
There was a problem hiding this comment.
SQInteger is the type used in every squirrel internal functions (especially sq_pushinteger() to pass a number to the squirrel VM and sq_getinteger() to get a number from the VM). It is indeed an int64, but I think using SQInteger is better as it hides implementation details from the API.
| { | ||
| return ScriptObject::GetRandomizer().Next(); | ||
| } | ||
|
|
||
| /* static */ uint32 ScriptBase::RandItem(int unused_param) | ||
| /* static */ SQInteger ScriptBase::RandItem(SQInteger unused_param) |
There was a problem hiding this comment.
unused_param doesn't really need to change.
There was a problem hiding this comment.
True, but my idea would be to use SQInteger everywhere in the API for generic numbers.
There was a problem hiding this comment.
Then, as a reminder for a later (cleanup) story: remove the casts from squirrel_helper_type.hpp:49-51 as well as 57.
There was a problem hiding this comment.
Ah you mean squirrel_helper.hpp
src/script/api/script_base.cpp
Outdated
| { | ||
| if (max < 0 || max > UINT32_MAX) { | ||
| ScriptLog::Warning("Clamping RandRange() parameter to 0..MAX(uint32)"); |
There was a problem hiding this comment.
ScriptLog::Warning is a bit too intrusive, better just tell a clamping may happen in the description, rather than outputing it into the log.
glx22
force-pushed
the
random
branch
2 times, most recently
from
0a342b7
to
19130eb
Compare
… and MAX(int32) Also ensure the parameters for ScriptBase::RandRange() and ScriptBase::Chance() are in [0-MAX(uint32)] range
There was a problem hiding this comment.
Should we add a note to the changelogs about the behavior being incorrect based on the specification in everything before OpenTTD 14? And that there is no compatibility layer that reintroduces this buggy behavior to scripts requesting an older API version?
I'm not sure though, as this is just fixing the behavior to be like it's specified. Though it might also break some expectations.
| { | ||
| return ScriptObject::GetRandomizer().Next(); | ||
| } | ||
|
|
||
| /* static */ uint32 ScriptBase::RandItem(int unused_param) | ||
| /* static */ SQInteger ScriptBase::RandItem(SQInteger unused_param) |
There was a problem hiding this comment.
Then, as a reminder for a later (cleanup) story: remove the casts from squirrel_helper_type.hpp:49-51 as well as 57.
… and MAX(int32) (OpenTTD#10443) Also ensure the parameters for ScriptBase::RandRange() and ScriptBase::Chance() are in [0-MAX(uint32)] range
Also ensure the parameters for ScriptBase::RandRange() and ScriptBase::Chance() are in [0-MAX(uint32)] range
Motivation / Problem
Documentation says ScriptBase::Rand() return a random value between 0 and MAX(uint32), but the actually returned value is between -MIN(int32) and MAX(int32).
ScriptBase::RandRange() has the same issue with values above MAX(int32) becoming negative.
Description
Use SQInteger as return type, so uint32 values are casted to int64 instead of int32.
Also use SQInteger for parameters, allowing to check them to be in the uint32 range.
Limitations
No compatibility scripts as the documentation was always correct.
Checklist for review
Some things are not automated, and forgotten often. This list is a reminder for the reviewers.