Commit 3b771b2
authored
Zero warnings: dish-windows (#89)
## Summary
The dish-windows part of the fleet-wide zero-warnings, zero-suppressions
pass. Every warning the three CI lanes printed on `main` (Windows CI,
CodeQL, Security) is fixed at its cause, every first-party `(void)`
discard and `NOLINT` is gone, the clang-tidy sweep now blocks, and the
tests build under `/WX` like the rest of the code.
## What was fixed, by category
**Compiler warnings (214 lines on the last `main` run of Windows CI, 9
in CodeQL)**
- `C5287` x 18 (9 per configuration): the cgutman ENet fork ORs an
`ENetProtocolCommand` with an `ENetProtocolFlag` in nine places.
`cmake/enet-msvc-c5287.patch` adds the `enet_uint8` casts the compiler
asks for (the assigned field is an `enet_uint8` already, so the runtime
bytes do not change) and `cmake/PatchEnet.cmake` applies it as the
FetchContent patch step, recognising an already patched tree because
ExternalProject re-runs the step on every update. Documented in
`THIRD_PARTY.md`; the `.patch` is pinned to LF in `.gitattributes`.
- `D9025` x 181 ("overriding '/Zi' with '/Z7'", every TU of `dish_core`
and `Dish` in Debug): the sentry block's raw `/Z7` sat on top of the
`/Zi` CMake bakes into the Debug flags. With CMake 3.25 (CMP0141) the
format is a target property, so `MSVC_DEBUG_INFORMATION_FORMAT=Embedded`
replaces the flag instead of overriding it. Same flags as before on
every target and configuration. The CMake floor moves from 3.21 to 3.25
(CMakeLists, presets, README).
- `C4244` x 4: the two ranged-for loops over brace lists in the tests
iterate a `std::array` of the element type.
**clang-tidy findings (20 on `main`, plus 1 that clang-tidy 22 adds)**
- `modernize-return-braced-init-list` x 7,
`bugprone-misplaced-widening-cast` x 5, `cert-err34-c` x 2
(`std::from_chars` replaces `atoi`), `bugprone-exception-escape` x 3 and
`performance-unnecessary-copy-initialization` x 1 in `MoonlightSession`
(the lambdas capture non-const movable copies through init-captures, so
the closures' implicit special members move instead of copy),
`readability-inconsistent-declaration-parameter-name` x 1,
`performance-no-automatic-move` x 1,
`bugprone-throwing-static-initialization` x 1 (`kFallbackUniqueId` is a
`constexpr QStringView`).
- `main.cpp`: the two pre-logger stderr lines go through
`QTextStream(stderr)`, the same bytes `fprintf` wrote, with no result to
discard (`cert-err33-c`) and no iostream failure path for
`bugprone-exception-escape` to report on `main`.
**Discards and suppressions removed (first-party)**
- 32 `(void)` discards: replaced by logging where the result carries
information (`dish.update.staging`, `dish.update.download`,
`dish.update`), by a returned value (`publishMotion`), by unnamed
parameters (`UsbDeviceGateway`), and in tests by assertions where the
value is known or plain calls where only the absence of a throw is under
test.
- 4 `NOLINT`s: the two empty catch blocks in the audio engines'
destructors now log through `dish.audio`; `SDLGamepadBridge.h` includes
`SDL_events.h` instead of forward-declaring SDL's reserved-identifier
struct tags.
- `_CRT_SECURE_NO_WARNINGS` and `_WINSOCK_DEPRECATED_NO_WARNINGS`
dropped from `dish_warnings`: both configurations build clean without
them.
- `-Wno-unused-command-line-argument` dropped from the tidy script: the
sweep reads a copy of the compile database with `/Zc:preprocessor`
removed (clang-cl's preprocessor conforms already, so the flag is
meaningless to it and its driver reported it as unused). Every other
MSVC flag clang-cl understands, so nothing is silenced.
- `/wd4702` on the whole `Dish` target narrowed to the
qmlcachegen-generated units it exists for (see "Left in place" below).
**Gates**
- `scripts/check-tidy.ps1` passes `--warnings-as-errors='*'` on the
command line, the way `linux-ci.yml` gates its sweep, so findings fail
CI while the fleet-canonical `.clang-tidy` (`WarningsAsErrors: ''`)
stays unforked. The script takes `-Files`, and the pre-commit hook runs
it over the staged sources in CI's file set, so a finding stops the
commit before it would stop the PR.
- Tests link `dish_strict` (`/WX`); Catch2's FetchContent declaration
takes `SYSTEM` (CMake 3.25) so its headers are external includes under
`-external:W0`.
- `codeql.yml`: the analyze step loses `continue-on-error` and `upload:
failure-only`, as its own comment said to do once the repository went
public (code scanning is active, 0 open alerts).
`CODEQL_OVERLAY_DATABASE_MODE=none` declares the mode the action fell
back to on every run after warning that a traced build cannot use
overlay analysis.
- `_security.yml`: dependency review loses its `continue-on-error` for
the same reason. That exposed the truth: the action fails with
"Dependency review is not supported on this repository. Please ensure
that Dependency graph is enabled", so it had never reviewed anything
here. `security.yml` now passes `dependency_review_enabled: false` with
the precondition spelled out (see below). `CONTRIBUTING.md`,
`SECURITY.md` and the PR template follow.
- `lukka/run-vcpkg` v11.5 -> v11.6 in all three workflows: it runs on
Node 24, which ends the "Node.js 20 is deprecated" notice for that
action. Nothing in its behaviour changed between the tags.
## qmllint: the last lowered level, closed
The one item this pass had left open. `scripts/check-qml.ps1` ran
`qmllint` with `--unqualified info` because `App` was a runtime context
property, so all 386 `App.x` reads in the QML tree came back as
`Unqualified access`, 278 diagnostics, and the category could not gate
anything. That level is back at its default, and `qmllint` over all 68
tracked QML files now reports nothing, in any category, at every
category's default level. No `// qmllint disable`, no pragma, no other
level touched.
`App` is a `Dish.Chrome` singleton now, spelled exactly as before in
QML. `AppViewModel` and the two role models stay in `dish_core`, which
links no Qml so the tests can keep exercising them without the Quick
stack, so they cannot carry `QML_ELEMENT` themselves;
`src/qml/chrome/ForeignTypes.h` declares their QML identities from the
`Dish` target with `QML_FOREIGN` instead, and
`qt_extract_metatypes(dish_core)` is what lets `qmltyperegistrar` read
their meta-objects and expand those declarations into the module's
generated `qmltypes`. That header is the whole of the new C++: 67 lines,
no logic.
Runtime behaviour is unchanged. `App` is registered by instance in
`QmlEntryPoint` exactly as `ChromeBridge`, `Theme` and `Tokens` already
are, for the reason those three already document: this target's LTCG
strips the generated `QQmlModuleRegistration` initializer, so a
declarative-only name would never reach the engine. The foreign
declaration's `create()` returns that same instance, so it cannot matter
which of the two registrations the engine resolves, and an explicit
`CppOwnership` keeps the engine from deleting a stack object.
One QML file changed, by one line: `shared/BindingDraft.qml` reads `App`
and was the only one of the 68 not already importing `Dish.Chrome` for
`Theme` and `Tokens`.
Verified past the linter, because the registration-stripping risk lives
in the shipping configuration: the Debug tree's 2229 tests pass, and the
Release build with LTCG, staged through `scripts/stage-bundle.ps1` and
launched from a scratch directory by `scripts/test-portable-bundle.ps1`,
stays up with an otherwise empty stderr. A singleton that failed to
resolve would print a binding error for every page that reads it, and
that gate reads stderr. `docs/QML_CONTRACT.md`, `docs/QML_UI_KIT.md` and
`CONTRIBUTING.md` no longer describe a gap that is not there.
## Left in place, with the reason
- `/wd4702` on the qmlcachegen-generated `*_qml.cpp` units only. MSVC's
optimizer reports unreachable code inside Qt's own `QJSEngine` and
`QVariant` templates (their `if constexpr` early returns) when the
AOT-compiled QML instantiates them; it fires in Release with and without
LTCG, and Qt 6.9 still has the pattern. `C4702` is a back-end warning,
which `/external:W0` cannot reach: Microsoft documents this under
`/external`, "Limitations", and names `/wd47XX` as the remedy. The only
alternatives change the shipped binary (`NO_CACHEGEN`, or `/Od` for
those units), so the option stays, scoped to the generated files;
`main.cpp`, the crash handler and the bridges keep `C4702` live.
- Dependency review: it needs the repository's Dependency graph, a
setting under Settings, Code security that I could not change (the
permission was denied, rightly). Enable it, flip
`dependency_review_enabled` to `true` in `security.yml`, and the job
blocks from then on.
- The `::warning::` the OSV-Scanner job prints on every run ("found no
package sources"): OSV-Scanner has no support for `vcpkg.json`, so the
job scans nothing in this repository; the shared security workflow
deliberately reports that rather than hiding it. Worth a fleet-level
decision (feed it an SBOM, or skip the job here), not a unilateral
change in this PR.
- `ilammy/msvc-dev-cmd` still declares Node 20 (no release targets Node
24), so its half of the deprecation notice remains; replacing the action
with a vswhere/vcvars import step is out of scope here.
- The release lane's best-effort `continue-on-error` steps (Sentry
symbol upload, Sentry release, Grype SARIF upload) are deliberate
release semantics and were not touched.
- `N warnings generated.` lines in the clang-tidy step's log are clang's
count of findings in Qt's and the STL's headers, which the header filter
drops; they are not findings.
## Judgment calls recorded
- CMake floor 3.21 -> 3.25 (November 2022): needed for CMP0141 and
FetchContent `SYSTEM`; the CI image and the documented local setup are
far newer.
- `dish.audio` (not `dish.audio.teardown`) for the new logging category,
following the `dish.<area>` convention of the existing categories.
- The RTSP `CSeq` parse accepts only a whole-value integer; `atoi` used
to return the digit prefix of a malformed value. Nothing but the tests
reads the field.
- The pre-commit hook now blocks on tidy findings (it used to be
advisory), matching the CI gate; it only ever ran when `core.hooksPath`
points at `.githooks`.
## Verification
- `cmake --preset debug` (fresh tree) + `cmake --build --parallel 8`:
637 steps, 0 warnings. `ctest --parallel --output-on-failure`: 2227/2227
passed.
- `cmake --preset release` from a wiped `build-release` (fresh ENet
population, patch applied: 2 + 6 + 1 casts in `host.c`, `peer.c`,
`protocol.c`) + build of `Dish` and `dish_setup_image`: 346 steps, 0
warnings, LTCG link included. The already populated debug tree was
patched in place on the next reconfigure, and the reverse check leaves a
patched tree alone.
- `scripts/check-tidy.ps1`: `clang-tidy: OK (100 files)`, 0 findings,
under the gate.
- `scripts/check-format.ps1` (clang-format 22.1.4): OK (495 files).
- `scripts/check-qml.ps1`: OK (68 files).
- No em-dashes in any added text; every diff is line-level (CRLF files
kept their endings).
## Review follow-ups (three commits on the same branch)
- `tests/test_moonlight_rtsp.cpp`: two cases pin the edges that
`std::from_chars` changed. A trimmed, whole-integer `CSeq` still counts
(padding, tabs, `INT_MAX`, a negative); a digit prefix with trailing
text, a leading `+`, an empty value or an overflow leaves the default,
where `atoi` answered the prefix or had no defined answer. `server_port`
reads the digit run after the key whatever follows it (`\r`, `;`,
letters); a space or sign directly after the key, or a run that does not
fit an `int`, is no port.
- `CMakeLists.txt` + `CONTRIBUTING.md`: the D9025 fix only held for a
fresh tree. CMake fills `CMAKE_<LANG>_FLAGS_<CONFIG>` once, so a
`build/` configured before the 3.25 floor keeps `/Zi` in its cached
Debug flags, and on that tree the Embedded property put `/Z7` on top of
it again (verified in place: 3.21-configured tree, reconfigured after
the bump, D9025 back). The configure now refuses such a tree with the
one-time remedy, delete its `CMakeCache.txt`. A fresh tree passes in
Debug, Release and RelWithDebInfo; CI trees are fresh on every run (only
the vcpkg binary cache persists), so nothing changes there.
- `tests/CrtAssertToStderr.cpp`: the last `[[maybe_unused]]` in
first-party code is gone; the hook object exists for its constructor,
which no compiler here warns about.
Re-verified on the final tip: Debug build 0 warnings, `ctest` 2229/2229
(2227 + the two new cases), `check-tidy.ps1` OK (100 files, and the gate
was shown to fail on a planted finding), `check-format.ps1` OK (495),
`check-qml.ps1` OK (68).
---------
Co-authored-by: Emir Hasanbegovic <1190336+emir-hasanbegovic@users.noreply.github.com>1 parent 7cb8c2c commit 3b771b2
56 files changed
Lines changed: 664 additions & 227 deletions
File tree
- .githooks
- .github
- workflows
- cmake
- docs
- scripts
- src
- Input
- Network
- core
- moonlight
- wire
- qml
- chrome
- repository
- source
- audio
- system
- usb
- update
- tests
Some content is hidden
Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
15 | 15 | | |
16 | 16 | | |
17 | 17 | | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
18 | 21 | | |
19 | 22 | | |
20 | 23 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1 | 1 | | |
2 | 2 | | |
3 | | - | |
4 | | - | |
5 | | - | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
6 | 6 | | |
7 | 7 | | |
8 | 8 | | |
| |||
30 | 30 | | |
31 | 31 | | |
32 | 32 | | |
33 | | - | |
34 | | - | |
35 | | - | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
36 | 38 | | |
37 | 39 | | |
38 | 40 | | |
39 | 41 | | |
40 | 42 | | |
41 | 43 | | |
42 | 44 | | |
43 | | - | |
| 45 | + | |
| 46 | + | |
44 | 47 | | |
45 | 48 | | |
46 | 49 | | |
47 | | - | |
48 | | - | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
49 | 54 | | |
50 | 55 | | |
51 | 56 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
14 | 14 | | |
15 | 15 | | |
16 | 16 | | |
17 | | - | |
| 17 | + | |
18 | 18 | | |
19 | 19 | | |
20 | 20 | | |
21 | 21 | | |
22 | 22 | | |
23 | 23 | | |
24 | 24 | | |
25 | | - | |
| 25 | + | |
26 | 26 | | |
27 | 27 | | |
28 | 28 | | |
| |||
37 | 37 | | |
38 | 38 | | |
39 | 39 | | |
40 | | - | |
41 | | - | |
42 | | - | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
43 | 44 | | |
44 | 45 | | |
45 | 46 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
206 | 206 | | |
207 | 207 | | |
208 | 208 | | |
209 | | - | |
210 | | - | |
| 209 | + | |
| 210 | + | |
211 | 211 | | |
212 | | - | |
213 | 212 | | |
214 | 213 | | |
215 | 214 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
43 | 43 | | |
44 | 44 | | |
45 | 45 | | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
46 | 51 | | |
47 | 52 | | |
48 | 53 | | |
| |||
89 | 94 | | |
90 | 95 | | |
91 | 96 | | |
92 | | - | |
| 97 | + | |
93 | 98 | | |
94 | 99 | | |
95 | 100 | | |
| |||
115 | 120 | | |
116 | 121 | | |
117 | 122 | | |
118 | | - | |
119 | | - | |
| 123 | + | |
120 | 124 | | |
121 | 125 | | |
122 | 126 | | |
| |||
127 | 131 | | |
128 | 132 | | |
129 | 133 | | |
| 134 | + | |
| 135 | + | |
| 136 | + | |
130 | 137 | | |
131 | | - | |
132 | | - | |
133 | | - | |
134 | | - | |
135 | 138 | | |
136 | 139 | | |
137 | 140 | | |
138 | | - | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
38 | 38 | | |
39 | 39 | | |
40 | 40 | | |
41 | | - | |
| 41 | + | |
42 | 42 | | |
43 | 43 | | |
44 | 44 | | |
| |||
178 | 178 | | |
179 | 179 | | |
180 | 180 | | |
181 | | - | |
| 181 | + | |
182 | 182 | | |
183 | 183 | | |
184 | 184 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
29 | 29 | | |
30 | 30 | | |
31 | 31 | | |
32 | | - | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
33 | 37 | | |
34 | 38 | | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
85 | 85 | | |
86 | 86 | | |
87 | 87 | | |
88 | | - | |
| 88 | + | |
89 | 89 | | |
90 | 90 | | |
91 | 91 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1 | | - | |
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
2 | 6 | | |
3 | 7 | | |
4 | 8 | | |
5 | 9 | | |
6 | 10 | | |
7 | 11 | | |
8 | 12 | | |
9 | | - | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
10 | 16 | | |
11 | 17 | | |
12 | 18 | | |
| |||
37 | 43 | | |
38 | 44 | | |
39 | 45 | | |
40 | | - | |
41 | | - | |
42 | | - | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
43 | 50 | | |
44 | 51 | | |
45 | 52 | | |
| |||
52 | 59 | | |
53 | 60 | | |
54 | 61 | | |
55 | | - | |
56 | | - | |
57 | | - | |
58 | | - | |
59 | | - | |
| 62 | + | |
60 | 63 | | |
61 | 64 | | |
62 | 65 | | |
| |||
116 | 119 | | |
117 | 120 | | |
118 | 121 | | |
119 | | - | |
| 122 | + | |
| 123 | + | |
| 124 | + | |
| 125 | + | |
| 126 | + | |
| 127 | + | |
| 128 | + | |
| 129 | + | |
| 130 | + | |
120 | 131 | | |
121 | 132 | | |
122 | 133 | | |
| |||
563 | 574 | | |
564 | 575 | | |
565 | 576 | | |
| 577 | + | |
| 578 | + | |
| 579 | + | |
| 580 | + | |
| 581 | + | |
| 582 | + | |
| 583 | + | |
| 584 | + | |
| 585 | + | |
566 | 586 | | |
567 | 587 | | |
568 | 588 | | |
| |||
589 | 609 | | |
590 | 610 | | |
591 | 611 | | |
592 | | - | |
593 | | - | |
594 | | - | |
| 612 | + | |
| 613 | + | |
| 614 | + | |
| 615 | + | |
| 616 | + | |
| 617 | + | |
595 | 618 | | |
596 | | - | |
597 | | - | |
| 619 | + | |
| 620 | + | |
| 621 | + | |
| 622 | + | |
| 623 | + | |
| 624 | + | |
| 625 | + | |
| 626 | + | |
| 627 | + | |
| 628 | + | |
| 629 | + | |
| 630 | + | |
| 631 | + | |
| 632 | + | |
| 633 | + | |
| 634 | + | |
| 635 | + | |
| 636 | + | |
| 637 | + | |
| 638 | + | |
598 | 639 | | |
599 | 640 | | |
600 | 641 | | |
| |||
613 | 654 | | |
614 | 655 | | |
615 | 656 | | |
616 | | - | |
| 657 | + | |
| 658 | + | |
| 659 | + | |
617 | 660 | | |
618 | 661 | | |
619 | 662 | | |
| |||
663 | 706 | | |
664 | 707 | | |
665 | 708 | | |
666 | | - | |
667 | | - | |
| 709 | + | |
| 710 | + | |
| 711 | + | |
| 712 | + | |
| 713 | + | |
| 714 | + | |
| 715 | + | |
| 716 | + | |
668 | 717 | | |
669 | | - | |
| 718 | + | |
| 719 | + | |
| 720 | + | |
670 | 721 | | |
671 | 722 | | |
672 | 723 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1 | 1 | | |
2 | 2 | | |
3 | | - | |
| 3 | + | |
4 | 4 | | |
5 | 5 | | |
6 | 6 | | |
| |||
0 commit comments