Hoist CropAndResize index offset to a named int64_t#29623
Open
titaiwangms wants to merge 1 commit into
Open
Conversation
Replace the inline `static_cast<int64_t>(...)` around the per-channel base offset in CropAndResizeForward with a named `int64_t bottom_data_offset` local in both the bilinear and nearest branches. Initializing an `int64_t` directly from the `SafeInt<int64_t>` expression is unambiguous and keeps the SafeInt overflow checking on the offset computation, while reading more clearly than the previous inline cast. No behavior change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Tita Wang <titaiwang@microsoft.com>
This file contains hidden or 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
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.
Description
Follow-up to the merged #29605, addressing a review nit from @apsonawane on the base-offset index arithmetic in
CropAndResizeForward.The suggestion was that the
static_cast<int64_t>(...)around the per-channel base offset looked redundant. It turns out the cast could not simply be removed: the offset expression is aSafeInt<int64_t>, andpointer + SafeInt<int64_t>is ambiguous —SafeIntexposes many implicit conversion operators (char,short,int, …), so the operand for pointer arithmetic cannot be resolved unambiguously (andSafeInt's ownoperator+(U, SafeInt)tries to treat the pointer as an integer). The cast was therefore load-bearing.To honor the readability intent without the awkward inline cast, this change hoists the offset into a named
int64_tlocal in both the bilinear and nearest branches:Initializing an
int64_tdirectly from theSafeInt<int64_t>expression is unambiguous (it selectsoperator int64_t()), so this compiles cleanly, keeps theSafeIntoverflow checking on the offset computation, and reads more clearly than the inline cast. No behavior change.Motivation and Context
Improves readability of the index arithmetic introduced in #29605 while keeping the overflow-checked offset computation intact.
Tests
Covered by the existing
CropAndResizeTestsuite — all 10 tests pass: