[BUG] array.h mixes signed and unsigned ints, causing potential issues

[MillaFleurs]

Describe the bug
The following function adds a signed + unsigned number together without explicitly casting, causing potential issues.

// Line 108
  /** The number of dimensions of the array. */
  size_t ndim() const {
    return array_desc_->shape.size();
  }

// Line 118
  /**
   *  Get the size of the corresponding dimension.
   *
   *  This function supports negative indexing and provides
   *  bounds checking. */
  auto shape(int dim) const {
    return shape().at(dim < 0 ? dim + ndim() : dim);
  }

To Reproduce

dim is signed ndim is unsigned so there's the possibility of unexpected behavior as the below code demonstrates.

// test.cpp
#include <cstdio>
#include <cstddef>
#include <limits>
#include <initializer_list>

int main() {
  std::size_t ndim = 3;

  for (int dim : {-1, -2, -3, -4}) {
    std::size_t idx = (dim < 0) ? (ndim + dim) : static_cast<std::size_t>(dim);
    std::printf("ndim=%zu dim=%d -> idx=%zu\n", ndim, dim, idx);
  }

  std::printf("SIZE_MAX=%zu\n", std::numeric_limits<std::size_t>::max());
  return 0;
}

// compile and run using the following command:
// clang++ -std=c++20 -Wall -Wextra -Wsign-conversion test.cpp && ./a.out

Expected behavior
This is a quick patch and I'm not sure why this wasn't caught in linting. As you can see if comiled with -Wall it comes up as a warning when this happens.

Desktop (please complete the following information):

• OS Version: Tahoe 26.2

[MillaFleurs]

Currently working on this

[zcbenz]

As the title says, it is potential but not really causing issues, because the at() method checks bounds. Explicit casting does not fix anything, and explicit boundary check only adds overhead.

[MillaFleurs]

@zcbenz you make a very interesting point and indeed at() already throws an out of range. However, the test I included failed prior to applying the patch I made, and this triggers -Wsign-conversion (and becomes a hard failure under -Werror). I’m aiming to keep the code warning-free under stricter builds.

I would also note that the intent here wasn’t memory safety but avoiding signed/unsigned arithmetic (dim + ndim()) and making negative-index normalization explicit / warning-free. If the helper hurts readability, I can keep it inline and rewrite the expression to avoid signed+unsigned mixing.

Thoughts?

[zcbenz]

For the signed/unsigned warnings we currently prefer switching to use signed types instead, work is currently tracked at #2723.

[MillaFleurs]

Thanks — I hadn’t seen #2723 yet (still getting familiar with the codebase). Agreed that moving sizes/indices to signed types (e.g., int64_t) is the right systematic fix and would make this local normalization unnecessary once that lands. Happy to defer/adjust this change to match the direction of #2723.

If you’d prefer I close this PR and instead help with #2723 (or add a small commit there addressing this call site), I’m happy to do that.

[zcbenz]

For the unsigned to signed type change we want to do it in one big PR rather than progressive small commits because it is going to affect almost every file. So I think we should just wait for #2723 (author is currently overloaded with other things).

[awni]

This was fixed in #2994 so closing for now.