Conversation
| // Update dimensions | ||
| Internal::width = width; | ||
| Internal::height = height; | ||
| if (!renderer || !renderer->Resize(width, height)) |
There was a problem hiding this comment.
what pixel buffer is the caller supposed to use when the renderer->Resize call fails?
There was a problem hiding this comment.
Good question. Here are the cases I currently see:
Before this change:
- If
reallocsucceeds butrenderer->Resizefails,SetSize()returns nullptr. The original pixel buffer is (potentially) invalidated, but the caller doesn't receive the new pointer. Problem! - If
reallocfails,SetSize()returns nullptr. The original pixel buffer is technically still valid, but Thirteen's internal pointer to the pixel buffer is now nullptr due to realloc's return value. Also a problem.
After this change:
- If
reallocsucceeds butrenderer->Resizefails, the result is the same as above (caller doesn't get new pointer - same problem) - If
reallocfails,SetSize()returns nullptr. However, Thirteen's internal pointer is unchanged, and the original pixel buffer is technically still valid.
In both cases, the caller will receive nullptr when anything fails inside SetSize(). I think it would be up to the caller to track that failure and try again. After this change, the old pixel buffer remains valid (as long as the caller doesn't immediately overwrite it).
However, a refactor that ensures both procedures (realloc and renderer->resize) succeed before updating the pixel buffer may be worth investigating?
There was a problem hiding this comment.
I agree. It's unclear what the user should do on failure other than terminate, but that isn't anything new to this change.
IMO the intent from my side was that the user should treat it as a fatal problem. If anyone has better ideas, let's improve it! (in a different PR)
Realloc in SetSize( ) was throwing warning C6308 in MSVC because it could potentially return null. Using a temp result and checking it before overwriting the Pixels pointer silences the warning.
Due to this change, the original Pixels array will remain after a realloc failure (as will the original width & height values).
(Also added my name to the credits! Thanks!)