Conversation
The clippy job runs on Linux only, so it compiles the cfg(unix) side of
every platform conditional and never sees the cfg(windows) side. Windows
-gated code was outside the warnings-as-errors gate entirely, not merely
under-linted: a lint could land there and no check in the set could fail.
Add both of the Linux job's clippy invocations to windows-build-and-test,
before the build so a lint failure costs seconds rather than the full
build/test/package cycle. That job is already a required check, so the
gate becomes load-bearing without a branch-protection change.
Turning it on surfaced 21 existing violations, fixed here. Most are
mechanical: `&mut x` passed to a Win32 call becomes `&raw mut x`, which
also avoids materialising a reference just to take a pointer from it;
`if c { 1 } else { 0 }` becomes `i32::from(c)`; `map(..).unwrap_or(..)`
becomes `map_or_else`/`is_ok_and`.
The rest are `missing_const_for_fn` and `unused_async` on platform stubs,
and those are deliberately silenced rather than "fixed". Making only the
off-unix arm of a cfg-split pair `const` would give the two platforms
different signatures — two of these are `pub` — and would push the same
lint onto every caller in turn, which is how the burn-down kept growing.
The `async` stubs must keep their signature because callers `.await` them
either way. Each site carries the reason inline.
Verified by cross-compiling the whole workspace to x86_64-pc-windows-gnu
(the msvc target cannot be checked off Windows: ring and aws-lc-sys need
a Windows C toolchain to build their build scripts). Both clippy
invocations are clean for that target and for Linux, and the test suite
passes.
Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
jussielo-amd
requested changes
Oct 2, 2026
jussielo-amd
left a comment
Collaborator
There was a problem hiding this comment.
Code review
Blocking
.github/workflows/ci.yml:590— The new "Clippy (Windows targets)" step runs twocargo clippycommands in one pwsh script with no error handling. pwsh defaults to$ErrorActionPreference: Continue, and GitHub Actions reads the step's pass/fail from$LASTEXITCODEat the end of the script — which only reflects the last command. So if the first (broader) clippy invocation fails but the second (narrower,e2e-cucumber-only) one passes, the whole step reports green. The entire point of this PR is to lint-gatecfg(windows)code, and this bug means it can silently fail to do that. The same file's "Probe sccache cache backend" step already handles this correctly with explicit$LASTEXITCODEchecks, so this isn't a tooling limitation — just a gap in the new step.
Worth raising
ci.yml:590— Runningcargo clippy --workspace --all-targetsimmediately beforecargo build --workspace --all-targetsin the same job causes Cargo to recompile the workspace twice (clippy'sRUSTC_WORKSPACE_WRAPPERchanges the build fingerprint, sobuilddoesn't reuse clippy's artifacts). Risks eating into the job's ~13min/45min timeout headroom the PR itself cites.ci.yml:590— This new lint step runs serially insidewindows-build-and-test(the repo's slowest job), rather than following the existing convention (seepowershell-lint) of a separate parallel job — so it adds to critical-path time instead of overlapping with the Linuxclippyjob, and a Windows-lint failure surfaces buried inside a build/test job rather than its own check.Cargo.toml:74— The workspace already has a[workspace.lints.clippy]allow-list mechanism for exactly this kind of structural false-positive (nursery lints on cfg-split platform stubs), but the PR hand-applies#[allow(...)]with duplicated justification comments to 9+ individual functions instead of one workspace-level policy change. Future platform stubs will keep re-triggering this and require copy-pasting the same idiom.
Minor
crates/rocm-core/src/uv.rs:283— The justification comment is copy-pasted near-verbatim across 9 files; one of them (make_executable) already describes a structurally different situation using near-identical wording, so drift has already started.ci.yml:593— The two clippy invocations run sequentially with no data dependency forcing that.apps/rocm/src/main.rs:3492—#[allow(clippy::missing_const_for_fn)]is applied unconditionally to a function that isn't actually cfg-split (just has one non-const branch), unlike every other suppression in this PR — if that branch ever becomes const-compatible, the suppression would stay in place with nothing forcing re-evaluation.
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.
What was wrong
The
clippyjob runs on Linux only. It therefore compiles thecfg(unix)side of every platform conditional and never sees thecfg(windows)side.That is not "Windows code is under-linted" — it is outside the warnings-as-errors gate entirely. A lint could land in Windows-gated code and no check in the set could fail, because no check ever compiled it.
windows-build-and-testbuilds and tests on Windows, so type errors are caught there; lints are not.What this does
Adds both of the Linux job's clippy invocations to
windows-build-and-test, before the build so a lint failure costs seconds rather than the full build/test/package cycle — the same reasoning behind that job's existingneeds: clippy.It goes in the existing job rather than a new one on purpose:
windows-build-and-testis already a required status check, so the gate becomes load-bearing immediately with no branch-protection change. That job currently runs ~13 minutes against a 45-minute timeout, so there is headroom.The 21 violations it surfaced
Most are mechanical and are fixed here:
&mut xpassed to a Win32 call →&raw mut x(also avoids materialising a reference just to take a pointer from it)if c { 1 } else { 0 }→i32::from(c)map(..).unwrap_or(..)→is_ok_and(..),map(..).unwrap_or_else(..)→map_or_else(..)use_self,uninlined_format_argsThe rest are deliberately silenced rather than "fixed", and that is the part worth reviewing.
Eleven are
missing_const_for_fnorunused_asyncon platform stubs. Applying Clippy's suggestion there is wrong in two ways:constgives the two platforms different signatures. Two of these arepub, so a const-context call would compile on Windows and fail on Linux.process_start_ticksconst pushed the same lint ontoProcessIdentity::capture; makingprobe_usable_amd_gpu_indicesconst pushed it ontousable_amd_gpu_indices. This is why the burn-down grew on each pass rather than shrinking.The
asyncstubs are the same shape: callers.awaitthem on both platforms, so droppingasyncwould not compile. Each site carries its reason inline. The one case whereconstis applied isWindowsExamineInventory::is_empty, which iscfg(windows)-only with no sibling to disagree with.Verification, and its limit
Cross-compiled the whole workspace to
x86_64-pc-windows-gnuwith mingw. Both clippy invocations are clean for that target, both are clean for Linux (no regression), andcargo test --workspace --all-targetspasses.The
msvctarget could not be checked locally:ringandaws-lc-sysneed a Windows C toolchain to build their build scripts, which does not exist off Windows. Thecfg(windows)code is identical between the two targets, but an msvc-only lint would appear for the first time in this PR's own CI run — which is, conveniently, exactly the gate being added. If one shows up, it is a real finding, not a flake.Clippy also stops at the first failing crate, so this converged over several passes (e2e-cucumber → rocm-core → rocmd → apps/rocm). The final pass reports zero.
Scope
CI config plus the lint fixes it requires — no behaviour change. No Gherkin scenario: nothing user-observable changes. The
&raw mutrewrites are the only edits insideunsafeFFI blocks, and they are the compiler-suggested form for taking a raw pointer without an intervening reference.