Repository navigation
Correct stale cross-platform twin comments - #1531
Conversation
c928228 to
869e37a
Compare
|
Reviewed, @bhelm. Every correction here is right, and I checked each one against the tree rather than taking the description's word for it. VerifiedComment-only confirmed mechanically — no
Doc lint clean (1,717 files), The
|
| Domain | Swift CaptureCompleteness |
Kotlin ReportCompleteness |
|---|---|---|
| import | ["import stage=", "rowsIn="] |
"import parser=" |
| battery | ["bank soc=", "socSeries"] |
"battery series=" |
Neither Kotlin token appears anywhere in the Swift map. Different shape too — Swift holds an array per domain, Kotlin one token plus a separate alternates map.
But the falsehood is now mirrored on the Android side, which this PR does not touch. ReportCompleteness.kt still says:
- line 4 — "Kotlin twin of the Swift
ReportCompleteness" — Swift has no such type; it isCaptureCompleteness - line 12 — "byte-identical to the Swift twin" — demonstrably not
- line 20 — "a parity test pins the map against the Swift twin"
And that parity test is the real find. ReportCompletenessParityTest.killerTokenMapIsTheCanonicalContract asserts "Byte-identical to the Swift ReportCompleteness.killerTokens" — then pins Kotlin against hardcoded literals, two of which do not exist in Swift. It never reads the Swift side, so it cannot fail when the platforms drift, which is the only thing it exists to catch. It has been green throughout the divergence it was meant to prevent.
None of that is a blocker for this PR — you corrected the side you set out to correct, and correctly. But merging leaves the two files openly contradicting each other, so it is worth a follow-up: fix the three Android lines, and either make that test genuinely cross-platform or rename it so it stops implying a guarantee it does not provide.
Happy to take the follow-up myself if you would rather not — say the word. Merging this as-is.
…uarded (#1564) Follow-up to #1531. @bhelm corrected the Swift side's false claim that the two completeness guards are kept aligned by a parity test. The same falsehood was mirrored on the Android side, which that PR did not touch, so the two files were left openly contradicting each other. ReportCompleteness.kt claimed its token map was "byte-identical to the Swift twin", and named a Swift type -- ReportCompleteness -- that does not exist; Swift's is CaptureCompleteness. Measured against the ten tokens the test pins: four match Swift exactly, three differ only by a trailing fragment, and three have no counterpart in the Swift map at all -- import ("import parser=" vs "import stage="), steps ("stepsEst " vs "stepsRaw") and battery ("battery series=" vs "bank soc="). That divergence is not a bug to fix. Each side must key on the tokens ITS OWN emitters write, and the two platforms word several of those lines differently, so aligning the maps would break the guard on whichever platform lost. The header now says so explicitly, with the three cases named, so nobody "corrects" them into alignment later. The test was the worse half. ReportCompletenessParityTest asserted "Byte-identical to the Swift ReportCompleteness.killerTokens" and then pinned literals held in the test file itself. It never read the Swift side, so it could not fail when the platforms drifted, and it stayed green throughout the divergence above. A test that names a guarantee it cannot check is worse than no test, because it invites a reader to stop looking -- and that is exactly what happened: the claim survived long enough for three tokens to lose their counterparts entirely. Renamed to ReportCompletenessContractTest and its doc rewritten to state what it actually guarantees, which is still worth having: Android's own map, section ordering and present/MISSING wording cannot change by accident. The cross-platform contract that DOES matter -- the section a maintainer reads being identically shaped -- is now called out as the real one. Comments and the class rename only; verified mechanically that the sole non-comment diff line is the class name itself. All 17 tests pass under the new name with no stale references remaining; Android 4241 tests, 0 failures matching main; doc lint clean. No Swift touched, so app-build does not apply.
Summary
HrvAnalyzer.sdnnIndextwin annotation after #1535 resolved the former platform gapThis is documentation-only: no declaration, runtime, test, schema, or workflow behavior changes. It makes the existing cross-platform annotations truthful and machine-resolvable without pretending differently shaped platform code is identical.
Verification
python3 Tools/doc_comment_lint.py— 1,717 files, cleangit diff --checkContext: bhelm/noop#17. The former Android metric gap in bhelm/noop#18 is resolved by #1535, so this refreshed comment-only delta no longer describes it as an open divergence.