Add a smoke test for non-empty nullptr
Span
#107444
Merged
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.
The above bug occurred because a
Span
was created fromnullptr
with asize > 0
. This is incorrect, because it's promising access to a memory region that does not exist.We can catch this, at least for the
nullptr
case, by adding a smoke test. This should help us catch such bugs early in the future. The above bug could only have occurred inDEBUG
builds, so it would have been prevented by this test.The test is pretty cheap, so it shouldn't create performance problems for
DEBUG
. I'd still hesitate to add it toRELEASE
because correct code should not need it, and in hot loops, it may actually prevent vectorization. Plus, setting_len = 0
is unlikely to be the correct fix anyway (though it should be better than nothing).Note: Unfortunately, this change requires the function to be non-
constexpr
, becauseERR_PRINT
isn'tconstexpr
. This can be fixed in c++20 withstd::is_constant_evaluated
. Fortunately, we do not use this property yet, so for Godot 4.5 we can give it up. In 4.6, we're planning to upgrade to C++20, so we can re-addconstexpr
then.