Skip to content

fix(#757): six C4456 shadowed locals block the Windows test compile - #779

Merged
localai-bot merged 2 commits into
mainfrom
row/FIX-MSVC-C4456-SHADOW
Aug 14, 2026
Merged

fix(#757): six C4456 shadowed locals block the Windows test compile#779
localai-bot merged 2 commits into
mainfrom
row/FIX-MSVC-C4456-SHADOW

Conversation

@localai-bot

Copy link
Copy Markdown
Collaborator

Closes #757. The third Windows defect, and the first that is not from the LTX-2.5 lane.

Why it appeared today

Two stacked defects landed this morning — #664 (f8cbc2310, POSIX stat) and #720 (0011bedf0, M_PI). The honest limit recorded at the time was that "the lane has never gotten past the LIBRARY compile."

It does now. windows-msvc-vulkan fails at a later stage — compiling a test target:

tests\vt\test_backend_cross_device.cpp(514,20): warning C4456: declaration of 'cpu' hides previous local declaration
tests\vt\test_backend_cross_device.cpp(514,20): error C2220: the following warning is treated as an error

The line is from 822b3a2e15 (2026-08-07). It was simply unreachable until the library compiled.

The fix

The inner "Unbind flash layout" block re-declares six names the enclosing scope already owns — cpu, cq, cd, ck, cv, cslots at :514-518, hiding :451-455. Renamed with an f prefix, after the block's own comment. Nothing else moved.

No warning was disabled, no /W4 widened, no #pragma warning(disable) added — same disposition #664 and #720 took: the warning is correct and the source is wrong.

The instrument, because my first one was wrong

I wrote a Python scope-sweep to find any remaining shadowing. On the pre-fix file it found five pairs where MSVC found six — it could not see cv, the second declarator on line 517. A lower bound presented as a count, which is exactly the failure mode this repo keeps paying for.

So the check is g++ -Wshadow, which reproduces MSVC's C4456 at the same lines and columns, with a positive control on the pre-fix copy:

PRE-FIX   :514:20 cpu   :515:13 cq   :516:20 cd
          :517:26 ck    :517:37 cv   :518:28 cslots     → 6
POST-FIX                                                 → 0

That column-exact match is what makes a Linux proxy usable for an MSVC-only diagnostic — the same technique that validated #720.

Pure renaming, proved not asserted

  • mapping the new names back reproduces the pre-fix file byte-for-byte
  • token counts equal at 28466
  • exactly 18 tokens differ, all of them the renames

BUILD_EXIT=0, 0 errors, 0 ENOSPC. test_backend_cross_device 19 cases / 3 assertions, exit 0 — the assertion count is small by construction, not by regression: the RegisteredDevices() loops are empty on a CPU-only build.

Why Linux never said anything

-Wshadow is not enabled for the CPU or Vulkan builds, while MSVC's /W4 /WX makes C4456 an error. This is not an ignored warning — nothing ever emitted it. Whether to enable an equivalent locally is a real decision (it would need its own row) and is deliberately not taken here.

What this does not claim

windows-msvc-cpu is now failing later still — it compiles everything, links, and runs test_openai_api_server.exe, which exits non-zero. That is a fourth, distinct issue and this PR does not touch it. Progress so far: library compile → test compile → execution.

Related: #664, #720, #584 (windows-msvc-* skipped on main, so no baseline), #680 (the portability checker's own tests run in no workflow).

🤖 Generated with Claude Code

FOLLOWING_AGENTS_PROTOCOL

`windows-msvc-vulkan` fails compiling a TEST target with

  test_backend_cross_device.cpp(514,20): warning C4456: declaration of 'cpu'
    hides previous local declaration
  test_backend_cross_device.cpp(514,20): error C2220: warning treated as error

and five more on the same block. The inner "Unbind flash layout" scope
re-declares SIX names the enclosing scope already owns -- `cpu` `cq` `cd` `ck`
`cv` `cslots` at :514-518, hiding :451-455. Renamed with an `f` prefix, after
the block's own comment. Nothing else moved.

Not from the LTX-2.5 lane: the line is from 822b3a2 (2026-08-07). It was
simply UNREACHABLE until the library compiled. #664 (f8cbc23) and #720
(0011bed) landed today and the Windows lane now gets past the library into
the test targets for the first time, which is what exposed it.

THE INSTRUMENT, because my first one under-reported. A hand-written Python
scope-sweep found FIVE shadow pairs where MSVC found six -- it could not see
`cv`, the SECOND declarator on line 517. A lower bound presented as a count.
So the check is `g++ -Wshadow`, which reproduces MSVC's C4456 at the same lines
AND columns, with a positive control on the pre-fix file:

  PRE-FIX  :514:20 cpu  :515:13 cq  :516:20 cd
           :517:26 ck   :517:37 cv  :518:28 cslots   -> 6
  POST-FIX                                           -> 0

That column-exact match is what makes a Linux proxy usable for an MSVC-only
diagnostic; it is the same technique that validated the #720 `M_PI` fix.

PURE RENAMING, proved rather than asserted: mapping the new names back
reproduces the pre-fix file byte-for-byte, token counts are equal at 28466, and
exactly 18 tokens differ -- all of them the renames. `BUILD_EXIT=0`, 0 errors,
0 ENOSPC; `test_backend_cross_device` 19 cases / 3 assertions, exit 0
(the device loops are empty on a CPU-only build, so the count is small by
construction rather than by regression).

Why the class was invisible on Linux: `-Wshadow` is not enabled for the CPU or
Vulkan builds, while MSVC's /W4 /WX makes C4456 an error. Nothing ever emitted
it. Enabling it locally is a separate decision and is NOT taken here.

Closes #757.

Following-Agents-Protocol: true
AI-Assisted: true
Assisted-by: AGENT:claude-opus-5[1m] [claude-code]
@localai-bot

Copy link
Copy Markdown
Collaborator Author

Two corrections to my own PR body, and one coordination miss

A fresh review passed this and, in doing so, showed that two of my claims are stronger than the evidence supports. Correcting them here rather than leaving them in the record.

1. "Column-exact agreement with the MSVC log" overstates what is on file

The only MSVC output anywhere in the record is a single line(514,20) in #757. That one matches. The other five columns are corroborated by GCC only. Every windows-msvc run that would show the full set was still in progress. So the honest statement is: one column confirmed against MSVC, six against g++ -Wshadow, and the two agree where they overlap.

2. 19 cases / 3 assertions, exit 0 proves almost nothing about the block I changed

I flagged the count as "small by construction". The review measured what that actually means, and it is weaker than I implied:

  • RegisteredDevices() (test_backend_cross_device.cpp:84-96) enumerates only {kCUDA, kMETAL, kVULKAN, kXPU, kROCM}CPU is deliberately excluded
  • so on a CPU-only build every device loop is empty, and 66 of the file's 69 CHECK/REQUIRE never execute
  • and specifically: the renamed block contains zero assertions, its only output ref_comb being consumed inside one of those empty loops

The run proves the block compiles and does not crash. Nothing about semantics. What actually carries correctness here is the reverse-mapping proof — mapping the new names back reproduces the pre-fix file byte-for-byte, 28466 tokens each, exactly 18 differing and all of them renames. That is the evidence to rely on; exit 0 should not be read as verification.

3. I did not check #524, and #757 told me to

PR #524 (row/ENG-RELEASE-WINDOWS-512) fixes the identical six shadows, on the same lines, with different names (unbound_cpu / unbound_queue / …) and identical hunk ranges. git merge-tree exits 1 on tests/vt/test_backend_cross_device.cpp.

It conflicts loudly rather than merging into a duplicate, so nothing silently breaks — but that is luck, not diligence. Whichever lands second drops this hunk; #524 already conflicts with main on five other files and is rebasing regardless.

What the review added that I could not have claimed

Completeness, as a count rather than a lower bound: a whole-tree -Wshadow sweep, TU_ENTRIES=912 TU_EXAMINED=912 TU_FAILED=0, 131 unique sites, of which the C4456/C4457 class is 15 — my six, seven in tests the Windows lane never builds, and two in Windows-compiled library code (server_main.cpp:1226, whisper_audio.cpp:317) that are lambda captures where the outer name is not captured, so nothing is hidden and MSVC does not flag them.

And it calibrated the proxy instead of assuming it: -Wshadow is a strict superset of C4456/57/58/59 — it fires on both declarators of a multi-declarator line, which is exactly what my Python sweep missed, and it over-reports member-shadows MSVC ignores. Over-reporting cannot hide a C4456, so it is sound for completeness while not being a formal equivalence.

One more correction to #757 itself: I wrote that the file has six vt::Backend& cpu declarations. It has 25, and seven TEST_CASEs contain two each — but real scope analysis says only :514-518 shadow. The rest are siblings.

FOLLOWING_AGENTS_PROTOCOL

Main pinned to an immutable SHA, not the ref: this is a shared checkout and a
peer's fetch advances origin/main mid-operation.

Following-Agents-Protocol: true
AI-Assisted: true
Assisted-by: AGENT:claude-opus-5[1m] [claude-code]
@localai-bot
localai-bot merged commit 3921160 into main Aug 14, 2026
0 of 16 checks passed
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.

2 participants