Skip to content

Hoist CropAndResize index offset to a named int64_t#29623

Open
titaiwangms wants to merge 1 commit into
microsoft:mainfrom
titaiwangms:cleanup/crop-and-resize-redundant-cast
Open

Hoist CropAndResize index offset to a named int64_t#29623
titaiwangms wants to merge 1 commit into
microsoft:mainfrom
titaiwangms:cleanup/crop-and-resize-redundant-cast

Conversation

@titaiwangms

Copy link
Copy Markdown
Contributor

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 a SafeInt<int64_t>, and pointer + SafeInt<int64_t> is ambiguous — SafeInt exposes many implicit conversion operators (char, short, int, …), so the operand for pointer arithmetic cannot be resolved unambiguously (and SafeInt's own operator+(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_t local in both the bilinear and nearest branches:

const int64_t bottom_data_offset = (SafeInt<int64_t>(roi_batch_ind) * channels + c) * height * width;
const T* offset_bottom_data = bottom_data + bottom_data_offset;

Initializing an int64_t directly from the SafeInt<int64_t> expression is unambiguous (it selects operator int64_t()), so this compiles cleanly, keeps the SafeInt overflow 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 CropAndResizeTest suite — all 10 tests pass:

onnxruntime_provider_test --gtest_filter='CropAndResizeTest.*'

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant