This change was made in 359fe90 due to recommendations from tooling.
On its face this change makes sense since it removes a copy that
isn't always necessary. In practice it caused ergonomic issues due
to now being forced to make a copy of the render states when needed.
The performance gains of eliding this copy are unsubstantiated. We
have not done any profiling to measure its impact. For lack of such
measurements I'd rather err on the side of improved user experience.
If future benchmarks prove this copy is rather expensive then we
can reconsider removing it with that evidence in mind.
While I don't suspect there are any uninitialize variable bugs
present, it's still good to err on the side of safety and provide
an initial value nonetheless.
While you can't easily run these unit tests, it's easy enough to
still compile them so we don't need to go out of way to prohibit
this. CMake has some support for running tests on the target OS
so perhaps in the future we find a compelling way to run the test
suite on a mobile OS.
This silences an MSVC warning about lsbDelta and rsbDelta not being
initialized. Whether or not this fixes any bugs, it provides a
better user experience if nobody sees those warnings.
Original protection to UB here was to simply check if the produced index
is out of bounds, but that isn't entirely correct imo. What is correct
is to fire if either the x or the y go out of bounds.
This shouldn't cause the UB problem to reappear, and be a little more
"correct" from the user's perspective. I believe that the most correct
way would be to use a boolean return in the setPixel(), and a
std::optional and return None on a failed fetch for the getPixel()
function. I don't know if ya'll are willing to do this, but this seems
sufficient too. Atleast there is a clear place where the programs fails
to work.
