UCS/DATASTRUCT: Add ucs_array_for_each_index

[guy-ealey-morag]

What?

• Add ucs_array_for_each_index which is a variation of ucs_array_for_each that also retains the index of the element.
• Refactor applicable indexed array loops and add unit tests.

Why?

Code that needs both an array element and its index currently uses a manual loop with ucs_array_elem.
This helper removes that boilerplate and makes switching between indexed and non-indexed iteration straightforward.
This macro would be used in later PRs I'm currently working on too.

Before:

for (idx = 0; idx < ucs_array_length(&array); ++idx) {
    elem = &ucs_array_elem(&array, idx);
    /* ... */
}

After:

ucs_array_for_each_index(elem, idx, &array) {
    /* ... */
}

[svc-nvidia-pr-review]

🤖 Starting review — findings will be posted here when done.

[svc-nvidia-pr-review]

🤖 Starting review — findings will be posted here when done.

[svc-nvidia-pr-review]

🤖 Review complete — no issues found.

[svc-nvidia-pr-review]

🤖 Starting review — findings will be posted here when done.

[svc-nvidia-pr-review]

🤖 Review complete — no issues found.

[svc-nvidia-pr-review]

🤖 Starting review — findings will be posted here when done.

[svc-nvidia-pr-review]

🤖 Review complete — no issues found.

[svc-nvidia-pr-review]

🤖 Starting review — findings will be posted here when done.

[svc-nvidia-pr-review]

🤖 Review complete — no issues found.

[tvegas1]

can we not add ucs_carray_for_each_index() macro and here use something like below which would have benefit of only relying on index to get a fresh elem pointer:

#define ucs_array_for_each_index(_elem, _idx, _array) \
    for ((_idx) = 0; \
         ((_idx) < ucs_array_length(_array)) && \
         (((_elem) = ucs_array_elem(_array, _idx)), 1); \
         ++(_idx))

[guy-ealey-morag]

Using ucs_array_elem would add an unnecessary assert on every iteration, the fact that it's part of the loop let us skip this check safely.
Also it's simpler to rely directly on the carray variant like it's done in ucs_array_for_each.

[tvegas1]

assert is already there long time and if needed can be removed for that case

[guy-ealey-morag]

This implementation is a direct extension to the existing ucs_array_for_each and ucs_carray_for_each. It uses the same patterns, same assumptions and it works in a very similar way.

The idea is that anywhere that can use the existing ucs_array_for_each would be able to use ucs_array_for_each_index to also have the index available.

[svc-ucx]

🤖 CI Triage Agent — UCX PR (Coverity coverity devel on coverity_rh7) · commit 74865927

TL;DR: The Coverity gate failed with 1 new defect (NO_EFFECT) introduced by this PR's new gtest: ucs_carray_for_each_index(elem, idx, values, 0) expands to for (idx = 0; idx < 0UL; ...), an always-false unsigned "< 0" comparison. Fix by passing the zero length through a non-constant variable (or suppressing the check) in test/gtest/ucs/test_datatype.cc:1123.

Full analysis

Summary: coverity devel on coverity_rh7 (Azure build 133245) ended with ##[error]Coverity found 1 issues → build script exits non-zero because nerrors=1.

Root cause: The build and cov-analyze both completed successfully (737 TUs, analysis 00:24:00, no compile errors). The failure is purely the defect gate:

/__w/1/s/test/gtest/ucs/test_datatype.cc:1123
  Type: Macro compares unsigned to 0 (NO_EFFECT)
  unsigned_compare: This less-than-zero comparison of an unsigned value is never true. "idx < 0UL".

Line 1123 is new code from this PR:

size_t idx, expected_idx;
ucs_carray_for_each_index(elem, idx, values, 0) { ADD_FAILURE() << ...; }

ucs_carray_for_each_index (src/ucs/sys/compiler_def.h:256) expands to
for ((_idx) = 0, (_elem) = (_array); (_idx) < (_length); ++(_idx), ++(_elem)).
With idx of type size_t and a literal 0 length, the loop condition becomes the constant-false idx < 0UL, which Coverity flags as NO_EFFECT. The sibling test array_for_each_index (line 1147) is not flagged because there the length comes from a struct field (ucs_array_length), not a compile-time constant. Note the pipeline is gated on any Coverity defect count > 0, so this single cosmetic finding fails the job — nothing else in the log is broken (no timeouts, no gaps, cov-build reported "completed successfully").

Implicated commit: 7a41231 — "UCS/DATASTRUCT: Add ucs_array_for_each_index" (Guy Ealey Morag), the PR head 7486592...

File: test/gtest/ucs/test_datatype.cc:1123 (macro at src/ucs/sys/compiler_def.h:256)

Suggested fix: Keep the empty-array coverage but stop feeding the macro a compile-time constant 0, e.g.:

UCS_TEST_F(test_array, carray_for_each_index) {
    const int values[] = {3, 5, 7};
    const int *elem;
    size_t idx, expected_idx;
    /* Non-constant length, so the loop condition is not folded to "idx < 0" */
    volatile size_t empty_length = 0;

    ucs_carray_for_each_index(elem, idx, values, empty_length) {
        ADD_FAILURE() << "iterated over an empty array";
    }
    EXPECT_EQ(0u, idx);
    ...
}

(Using volatile — or a value read from a helper/member — reliably defeats Coverity's constant folding; a plain const size_t may still be folded.) If the team prefers to keep the literal, add a /* coverity[no_effect] */ annotation immediately above line 1123 instead. No change to ucs_carray_for_each_index itself is needed — the macro is correct; only the constant-0 call site is unanalyzable. Also fix the EXPECT_EQ(0, idx) signed/unsigned comparisons to 0u/size_t while touching this test.

Related: PR #11853 ("UCS/DATASTRUCT: Add ucs_array_for_each_index") — the PR under test; no pre-existing issue found for this defect.

[svc-nvidia-pr-review]

🤖 Starting review — findings will be posted here when done.

[svc-nvidia-pr-review]

🤖 Review complete — no issues found.