Pass 4a: the features lane duplication audit (#385) - #998
Merged
Conversation
The scope resolver is an agent, not a parser, and the only existence check was a sentence in its prompt telling it to test -f each path and drop the misses. That is a soft instruction to a model, and #385's own run ignored it: the recorded scope named four files that do not exist in this repo (Topology.swift, XCAF.swift, OCCTBridge_XDE.h/.mm), the audit ran to completion anyway, and its closing summary read as coverage. Workflow scripts have no filesystem access, so this cannot be an existsSync. It is a second, independent agent that does nothing but run test -f, and deliberately not the one that produced the list. A path counts as missing unless it is affirmatively reported as existing, so an incomplete verdict fails the same way a false one does. Verified in Scripts/repro/385-scope-guard/, which pulls the two shipped expressions out of the workflow by regex rather than copying them, and exercises ten cases: 10/10. Proven to matter first, origin/main's version of the file contains zero of these checks, which is why every scope above, including the fabricated one, was accepted. Refs #385
…compliance Done ahead of the Pass 4a sweep rather than inside a fix PR. This file backs 15 of the lane's 32 bridge calls, so the sweep will find things in it, and the style manifest is a one-way ratchet: whoever touches it first has to carry a 2,208-line reformat on top of their actual change. That is the shape that got #917 deferred out of PR #912, and deferring it again would stall the lane. Both files of the pair, since the .h was on the manifest too and a finding in one reaches the other. OCCTBridge_ProjLib_NLPlate.h 310 LOC, 345 diff lines OCCTBridge_ProjLib_NLPlate.mm 1255 LOC, 2208 diff lines Behaviour-neutral, verified rather than assumed: with comments and whitespace stripped, both files are byte-identical to their previous versions (5,748 and 30,337 code characters). The .mm is whitespace-only even with comments left in. swift build clean, swift test --filter "Plate|ProjLib" 69 tests in 24 suites passing, all eight gate scripts green. Removing a file from the manifest without actually reformatting it just moves the failure to code-style.yml's clang-format --dry-run --Werror, which is how main sat red for five merges (#942), so the removal and the reformat are the same commit. Refs #385
Contributor
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (20 files)
Previous Review Summaries (9 snapshots, latest commit 5a78502)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 5a78502)Status: No Issues Found | Recommendation: Merge Files Reviewed (380 files)
Previous review (commit e5aa1fa)Status: No Issues Found | Recommendation: Merge Files Reviewed (24 files)
Previous review (commit d0a1290)Status: No Issues Found | Recommendation: Merge OverviewThis incremental review (from commit
Changes Reviewed1. #1026 — Null
2. #1036 — Perspective eye anchor fix
3. #1037 — GD&T enum range checking (5 setters)
4. #1038 —
5. #996 — 6. Audit workflow scope guard (#385)
7. ProjLib_NLPlate clang-format ratchet
Minor Observations (Non-Blocking)The following are style/fragility notes in reproduction/infrastructure scripts only — not production code:
Test CoverageAll four new test suites pass Swift Testing conventions:
Gate ScriptsAll eight gate scripts pass including new Verification
Files Reviewed (42 files)
Previous review (commit f0857b7)Status: No Issues Found | Recommendation: Merge Files Reviewed (7 files)
Previous review (commit 8ddd271)Status: No Issues Found | Recommendation: Merge Files Reviewed (2 primary files + supporting changes)
Previous review (commit 5a6ad36)Status: No Issues Found | Recommendation: Merge Files Reviewed (1 file)
Previous review (commit e02f057)Status: No Issues Found | Recommendation: Merge Files Reviewed (48 files)
Previous review (commit 98cb92b)Status: No Issues Found | Recommendation: Merge Files Reviewed (3 files)
Previous review (commit b8b2d27)Status: No Issues Found | Recommendation: Merge Files Reviewed (5 files)
Reviewed by nemotron-3-ultra-550b-a55b:free · Input: 417.9K · Output: 6.6K · Cached: 2.1M |
Two screening detectors and the measurement they produced, kept because the question they answer keeps coming back and the answer keeps being re-derived by hand. Neither is a gate. Both carry their caveats in the README, including the two occasions they were wrong before they were right: a .mm-only scan reported BRepFeat_MakeCylindricalHole at 0% coverage against CLAUDE.md's own account of wrapping it, because the calls live in a shared helper in OCCTBridge_Internal.h; and the dead-parameter detector's first run counted 23 deliberate ABI no-op stubs among its 36 hits. Refs #385
…oing orthographic OCCTDrawingCreate declared an OCCTProjectionType nothing read, so every Drawing.project(..., type: .perspective) call returned the orthographic projection. It now selects HLRAlgo_Projector(gp_Ax2, Focus), which needs a focal distance, so ProjectionType carries one as an associated value rather than growing a separate focus: parameter that would be dead whenever the type is .orthographic. OCCTDrawingCreatePoly loses its projectionType instead of gaining a focus: measured on the pinned 8.0.1 kernel, HLRBRep_PolyAlgo stores the projector's perspective flag (readback confirms Perspective() == 1) and produces output identical to the orthographic projection at every focal distance, including one where HLRBRep_Algo diverges fourfold. Swift's projectFast never exposed the parameter, always passing a literal 0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#975) #974: OCCTShapeQuilt and OCCTShapeQuiltWithHistory fed BRepTools_Quilt with byte-identical loops and took its Shells() the same way. Both now call occtQuiltShells. The helper is file-static because BRepTools_Quilt has exactly those two call sites, both in OCCTBridge_Modeling.mm, and appears in no other .mm and no header. #975: four entry points open-coded "the first edge of this shape" as a bare-edge special case plus the first hit of a TopExp_Explorer, seven copies across two files. All four now call occtEdgeAt(shape, 0), the helper OCCTBRepExtremaExtCC in the same file was converted to by #613. Measured equivalent over eleven fixtures before the swap, same edge and same orientation on every one, Scripts/repro/975-first-edge-idiom/. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#994) OCCTBridge_Topology.mm's trsfFromMatrix12 / matrix12FromTrsf and OCCTBridge_BRepGraph.mm's locationFromMatrix each turned the same twelve doubles into the same gp_Trsf, 4,800 lines apart in two files. Two files, so the pair and the TopLoc_Location composite both call sites build live in OCCTBridge_Internal.h as inline rather than static in either. The layout is in the helper names. This bridge carries two 12-double conventions and #835 already separated them on the Swift side into TransformMatrix3D (INTERLEAVED) and Matrix12Grouped (GROUPED); the three GROUPED bridge sites keep their own permuted SetValues and are deliberately not converged onto these. Measured: a GROUPED array read as INTERLEAVED comes back as translation (0, 0, 7) instead of (5, 6, 7), accepted rather than refused, because this kernel's No_Exception build compiles SetValues' own orthonormality precondition out. Scripts/repro/994-matrix12-interleaved/ measures the two spellings agreeing bit for bit over five matrices and the round trip being exact. It also records that the BRepGraph half is unreachable from Swift: the location those four call sites write has no read-side API, and BRepGraph.shape(nodeKind:) returns the shape unplaced. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…f zero-filled fields The GD&T read surface existed twice: an untyped family on `Document` returning raw `Int32` type codes, and a typed family behind `typedDimension(at:)` and friends, both reading the same three bridge calls and rebuilding the same fields into different structs. There is now one family, the typed structs under the untyped family's method names. Both families also flattened OCCT's dimension kinds into one struct with zero-filled fields, which is a live wrong answer, not just duplication. `XCAFDimTolObjects_DimensionObject` encodes a dimension's magnitude in a values array whose length is the discriminator, and `OCCTDocumentGetDimensionInfo` read the first slot and called it `value`. For a range that is the lower bound, and both tolerance accessors answer a flat 0, so a 10..12 range read back as `value = 10, lowerTolerance = 0, upperTolerance = 0`, indistinguishable from a plain 10mm dimension with zero tolerance. Reproduced against the pinned kernel before fixing, transcript in Scripts/repro/996-gdt-read-surface/. `Document.Dimension.Bounds` mirrors OCCT's own predicates instead: `.unset`, `.simple`, `.range`, `.plusMinus`. `value` is `Double?` and `classOfTolerance` is optional, so nothing that OCCT reports as an inapplicable 0 is surfaced as a measurement (#609/#726). The class of tolerance is a separate optional rather than a fourth `Bounds` case, because it is stored outside the values array and a range dimension can carry one; measured both ways. `OCCTDocumentSetDimensionTolerance` now returns the conjunction of OCCT's two setter results. Both return false and change nothing for a dimension that is already a range, and discarding them reported success for a call that did nothing. `Scripts/derive-gdt-enums.py` is the drift gate the enums never had. Four hand-transcribed enums, 97 members, derived from the pinned headers into `Scripts/occt-gdt-enums.txt`; `--verify` compares Swift against that manifest and needs no kernel, so it gates in CI, and `--reverify-headers` re-derives after an OCCT bump. `--self-test` is 12 cases, every guard proven by removal. The remaining accessor surface (71 of 85 across the three XCAFDimTolObjects classes) is deferred to #1004.
The two duplicated helpers sit in different .mm files, so "4,800 lines apart" was a number with no referent. Say what is actually load-bearing instead: three call sites on one side, four on the other, which is the reach that decided the placement. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`orthonormalRadial(axis:)` built its own perpendicular with a `cross(axis, worldUp)` construction and a 0.9 magnitude threshold, beside the module-wide `perpendicularBasis(to:)` (#881) that already matches OCCT's `gp_Ax2(gp_Pnt, gp_Dir)` and is shared by `Placement`, `ConstructionPlane`, `Shape.sectionPlaneBasis` and `Drawing`. The two are not interchangeable, so the convergence is a choice of element: measured in `Scripts/repro/990-orthonormal-radial-basis/` against gp_Ax2 read from the pinned kernel, the old construction equals the second element on +X, +Y and +Z and is 180 degrees from it on -X, -Y and -Z. The second element is taken, so threads on the axes every test, doc and cookbook snippet uses keep the clocking they shipped with. New `Issue990ThreadAxisBasisTests` measures the groove's angular position on a real threaded rod for all six world axes against the gp_Ax2 constants.
Two findings in the same type, fixed together because both are the profile answering questions about itself. #988: `square` and `buttress` were literal six-vertex lists sitting next to `whitworth55`, `acme29` and `trapezoidalMetric30`, which are all built by the `trapezoid` factory. Both are trapezoids: `square` is the limiting one, crest and root flats each half the pitch, and `buttress` is the same construction with the crest flat off centre, which is what makes its two flanks differ. The factory gains a defaulted `crestCentreFraction` and both constants come from it. The vertices are bit-for-bit the ones the literal lists held, checked with `==` on `Double` rather than a tolerance. #991: the cut path's local `flatWidth(atDepth:)` closure re-derived from `spec.profile.segments` something only the profile knows, and it was the third spelling of "is this segment a flat at this depth" in the file. That predicate is now `Segment.isFlat(atDepth:)`, read by `hasCrestFlat`, by the direct build's crest lookup and by the new `ThreadProfile.flatWidthFraction(atDepth:)` the cutter now calls. Zero behaviour change on both counts.
`parseAcme` and `parseUnified` read the same `"<diameter>-<threads per inch>"` body, down to the `* 25.4` and `25.4 / tpi` conversions and the `tpi > 0` guard. Both now call `parseInchDesignation(_:form:)`, so ACME and Unified differ only in the suffix each strips and the form it stamps, and the five `25.4` literals across the file are one `mmPerInch`. The four prefix-and-table-lookup statements in `parsePipeOrWhitworth` become one ordered table, and the three size dictionaries share a `SizeTable` typealias. One measured behaviour change, on malformed input only: `parseAcme` used to require the whole tail after the hyphen to be the thread count, and now reads to the first space, since that is what `parseUnified` must do for `"1/4-20 UNC"`. `"1.5-4 x ACME"` is accepted where it was refused. Every well-formed designation parses to the same numbers as before. The other three parse methods are left alone: measured, they do not share the proposed prefix-and-components structure. New `Issue989ThreadDesignationParseTests` pins form, diameter and pitch for all fourteen recognised designations, thirteen refusals, and the two edge behaviours above.
… OCCT does Review on PR #1011 flagged the comment above this setter pair as stating a guarantee rather than a measurement. It said "OCCT rejects the pair together", which is true for the one rejection this project measured, a range dimension where both setters refuse, and is not documented upstream as a general property of every path. Two changes. The comment now says what was measured and says plainly that it is not a general guarantee. And the verdict reads the two values back rather than trusting the returned pair, so a partial application is reported as a failure instead of being indistinguishable from a no-op. clang-format clean, gates green, GD&T suites pass. Refs #996
refactor: one INTERLEAVED matrix-to-transform conversion in the bridge (#994)
fix(#990): take the thread datum from the shared perpendicular basis
…n-parse refactor(#989): one imperial designation parse, not two
…rawing fix(#999): Drawing.project honours .perspective instead of silently going orthographic
…lies (#995) OCCTBridge_Curve3D.mm and OCCTBridge_Surface.mm each declared buildTrsf3D at the top of the file and defined it next to their own in-place transform dispatcher, with byte-identical bodies serving seven call sites apiece. Two files, so it moves to OCCTBridge_Internal.h as inline occtBuildTrsf3D rather than staying static in either. The type codes are the bridge's own contract, documented on OCCTCurve3DTransform and referenced by OCCTSurfaceTransform, so the switch is unchanged. OCCTBridge_Geom2d.mm's buildTrsf2D stays static there: it builds a gp_Trsf2d from four doubles rather than a gp_Trsf from seven, and has one file's worth of call sites. Its two comment references are repointed. already runs inside a try, which matters because gp_Dir throws on a zero-length direction. Re-measured rather than inherited: all fourteen call sites still sit inside their own function's try. CLAUDE.md's sentence saying there are two static copies is corrected in place. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
refactor: one discriminated gp_Trsf builder for the 3D transform families (#995)
Five of the six built a default-constructed Draft_EdgeInfo/Draft_FaceInfo/ Draft_VertexInfo, read one property off the throwaway and returned it; the sixth echoed its argument back. Deletion rather than rewiring, and the measurement that settles it: the only OCCT class holding populated Draft_*Info objects is Draft_Modification, whose myFMap/myEMap/myVMap are private with no public accessor (verified against the pinned 8.0.1 headers). There is no public route to a populated instance, so "wire it to real objects" is not a new API, it is unreachable without patching the kernel. Two further header facts the issue could not have had: Draft_EdgeInfo declares no SetTangent at all (it has Tangent(const gp_Pnt&) and IsTangent), so OCCTDraftEdgeInfoSetTangent is named after a method that does not exist; and Draft_EdgeInfo has SecondPC, not LastPC. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
fix(#1000): delete DraftInfo, six members that never queried their input
One GD&T read family, and OCCT's dimension kinds instead of zero-filled fields (#996)
…, FeatureReconstructor - Add Placement.lift() method to ConstructionEntity.swift - Remove private Sketch.lift() and use Placement.lift() - Remove Flange.worldPoint() and add Placement property - Update FeatureReconstructor.applyExtrude to use Placement.lift() Closes #972
…, FeatureReconstructor - Add Placement.lift() method to ConstructionEntity.swift - Remove private Sketch.lift() and use Placement.lift() - Remove Flange.worldPoint() and add Placement property to Flange - Update FeatureReconstructor.applyExtrude to use Placement.lift() - Fix swift-format documentation comment issues in touched files - Remove touched files from style manifest (FeatureReconstructor.swift, SheetMetal.swift, Sketch.swift) Closes #972
Three things, from the pass owner's review. Flange.init normalised `normal` into its own stored property but passed the raw parameter into Placement(zAxis:), so flange.normal and flange.placement.zAxis disagreed for any non-unit input. Placement documents a unit zAxis on all three axes. Now passes the normalised `n`. `placement` is no longer public. Placement documents an orthonormal basis and this one is not: Flange lets a caller supply any uAxis/vAxis, by its own documented design, and lift has to keep scaling by them to match the worldPoint it replaced. Normalising xAxis is therefore not available as a follow-up, so the frame must not be published as one callers may treat as unit. The three call sites are all inside this file. A doc comment still referred to `worldPoints`, the method this PR deletes. Adds the tests the PR shipped without. The zAxis test was run once against the unfixed code and fails there with simd_length(d) = 6.0, for a normal of (0,0,7) against its normalised (0,0,1); it passes once restored. The two lift tests cover the formula and a rotated basis, so a transposed or dropped axis cannot pass. Gates green, swift-format clean, count-operations derives 4340 unchanged. Refs #972
…3d-lifting refactor(#972): deduplicate 2D→3D lifting logic in Sketch, SheetMetal, FeatureReconstructor
…teErrors OCCTSurfaceNLPlateG2/G3 declared a maxIter and called NLPlate_NLPlate::Solve2(ord, InitialConsraintOrder), which has no iteration count. IncrementalSolve does, but measurement says it is a different solver rather than a bound on this one: on a five-constraint G0G2 saddle it returns a surface 2% away by checksum and reports Continuity() 3 where Solve2 reports 1, while NbIncrements itself is inert from 2 upward. It is already wrapped separately as OCCTSurfaceNLPlateIncrementalG0. The parameter is removed rather than redirected. OCCTGeomPlateErrors is deleted rather than having its two dead parameters trimmed, because the three values it returned were never computed: GeomPlate_BuildPlateSurface assigns myG0Error/myG1Error/myG2Error only in VerifSurface(), which Perform() reaches only when there are curve constraints, and the members have no initialiser. This entry point is point-only by construction. Measured through the real bridge, three consecutive calls on one fixture returned 1.94e-313, -3.11e+231 and -nan, and no value moved with tolerance, maxDegree or maxSegments. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…Create's declaration The bridge header described focus as an eye-to-origin distance that must be positive, which was right as far as it went, but said nothing about the frame being anchored at the world origin or about the shape having to clear the eye plane. Both are now on the declaration, where a bridge caller reads them. Refs #385
The first rewrite of that comment claimed focus 0 and 1e-12 return a mirrored projection. Measured on the fixture the comment names, a 100x50x30 centred box viewed down +Z, that is false: all four of 0, 1e-12, 5 and 15 return an empty VCompound, exactly as originally written. The data was right and the inference was not, so the list is trimmed to the two values the focus > 0 test actually rejects, and the note now says what 5 and 15 were really measuring: that box spans z [-15, 15], so both put the eye inside it or on its face, which is the straddling case the new reach guard handles. Adds guard_comment_probe.mm so the claim is re-measurable rather than trusted. Refs #385
A sentence that lost its verb in editing, and a distance stated as 50 units where the measurement is 40: the eye at focus 50 over a box spanning z 0 to 10 sits 40 units from the near face at z = 10, which is what makes it comparable to the focus 1050 case over z 1000 to 1010. The half-widths quoted beside it (6.25 and 131.25) were always the measured ones and are unchanged. Refs #385
…obes Matches the convention in the neighbouring repro directories: the probe alone does not say what it measured or which decision the number supported. Records the three regimes, where the bounding-box guard's boundary lands, the translation-invariance measurement that argued against a caller-controlled eye position, and the re-measurement of the pre-existing guard comment. Refs #385
fix(#1026): guard the shape, not just the pointer, at 46 bridge sites
gsdali
changed the title
…manifests
okf/policies/writing-style.md bans the em-dash outright and says a dedicated
pass to strip the existing ones is not required. This is that pass.
10,614 replaced across 376 files. Not the 501 an earlier count reported: that
came from one agent's five-file scope, not from the repo, which held 10,932.
Three rules, derived from the measured distribution rather than chosen. 97.6%
were the spaced form, 1,761 lines matched the labelled-term template, and 909
dashes were followed by a capitalised word:
COLON after a labelled term at the head of a line, list item or table cell,
which is the docs/reference/ parameter template.
PERIOD where the line has exactly one dash and the next word is capitalised,
so the halves are independent clauses. Unpaired dashes only, so a
parenthetical is never split into sentences.
COMMA everything else.
The three exclusions on the period rule are measured failures of earlier
drafts, not precautions: a period landed inside a code span, producing
`contains(uid:). GraphUID`; another landed inside an unclosed parenthesis,
splitting an aside; and swift-format rejected `domain. NOT arc length` as a
two-sentence doc summary, which is the all-caps emphasis case.
318 em-dashes remain, in the 55 files on a style manifest, and they are left
deliberately. The ratchet requires a touched file to be brought fully clean and
delisted in the same PR. Measured: that is 12,421 reformat lines, and
swift-format format -i does not finish the job, since 264 lint violations
survive it. Seven are AlwaysUseLowerCamelCase on public enum cases including
PaperSize.A0 and A1, so cleaning those files means a source-breaking rename. A
punctuation sweep is not the change that should carry one. Each clears when
someone next edits that file for a real reason, which is what the ratchet is
for.
Verified rather than assumed: swift build clean, full swift test 5,780 tests in
1,489 suites green and unchanged, all eight gates green, swiftlint 0
violations, both style manifests clean, and swift-format lint / clang-format
--dry-run --Werror clean on every changed file.
Nothing behavioural moved. The replacements inside string literals are all
display text: deprecation messages, error descriptions and @Suite/@test names.
Checked before the sweep that no test asserts on any of it; the test hits are
#expect failure messages, and swift test --filter matches the struct name
rather than the display string.
Tool and its measurements kept at Scripts/repro/385-em-dash-sweep/.
Refs #385
… child each The measurement #1026's own README names as missing and defers to its own issue: what a null `TopoDS_Shape` actually does at each OCCT entry point the bridge hands one to. Same shape as `556-null-handle-guard-sweep` for a null geometry `Handle`, which is the precedent this repo set for deciding a guard on evidence. 17 uncatchable, 4 catchable, 40 return.
…cing entry point `TopoDS::Edge` is written `theShape.IsNull() ? false : ...` (`TopoDS.hxx:94`), so it deliberately passes a null shape through and the crash lands one frame further out, at `BRep_Tool::Curve`, a `BRepAdaptor_Curve` constructor or similar. That is why #1008's census of 345 cast sites came back clean while the operations built on those casts still died, inside a `try` that cannot catch an OS signal. Every guarded site already had a refusal for a null pointer or a wrong-typed input, and a null shape goes on that same path, so nothing is invented (#726). `OCCTShapeFixerCreate` had no null test of any kind and its declared return is `_Nonnull`, so its refusal is an empty `Handle`, which the seven accessors now test for rather than testing only the pointer. `occtShapeIsPresent` gains overloads for the wire, edge and face wrappers so a call site reads the same whichever of the four it guards.
…outward past a cast #1026's `SHAPE_BUILDER_TYPES` was the first measured member of the class "an OCCT entry point that dereferences the caller's shape for you", so it is folded into one table rather than left beside a second: `SHAPE_DEREF_RECEIVERS`, `SHAPE_DEREF_QUALIFIED` and `SHAPE_DEREF_CTORS`, one per spelling the bridge writes. Every entry is a probe from `repro_1035.mm`, never a guess. `enclosing_calls()` walks outward through the `TopoDS::` casts, which return a null of the narrower type rather than raising. Stopping at the innermost call is what hid thirty `Type local(TopoDS::Edge(x->shape))` sites from this PR's own first census, and finding them is what the outward walk is for. Five fixtures, `SL` to `SP`. Three are the false-positive direction, including `ShapeAnalysis_ShapeTolerance::Tolerance` taken verbatim from `OCCTShapeMaxTolerance`: `Tolerance` is in the table under `BRep_Tool` and this one is measured to cope, so without it removing the qualifier test changes no outcome, which is how matrix row R12 was first found decorative. Five existing fixtures gain a shape guard. They exercise the local-handle walk and were unguarded in a second, independent way the new table correctly reports; guarding the shape keeps each row isolating one mechanism. Matrix rows R8 and R9 are rewritten onto the new code and R10 to R13 added. Every row now drops at least one case.
chore: replace every em-dash with ordinary punctuation (10,932 across 431 files)
…e unfixed tree Run one process each, because the failure mode is an uncatchable signal and one crash in a shared process hides every test after it. Against the unfixed tree ten report `signal code 11` and both controls pass; against the fix all twelve pass. The first control caught a fixture of this PR's own that had stopped meaning its name: it asserted `extrudedInfinite` on the solid, which legitimately returns nothing, so it failed for a reason with nothing to do with the guard. It asserts a face now, which is a profile `BRepPrimAPI_MakePrism` actually extrudes.
…opted Records the re-measured sweep, the fallback-divergence number the issue asked for and nobody took (2, not 904), the entry-point table, and each of the 39 functions without a `try` with what was decided for it. The accessor is declined on the thirteen where a null shape is the function's own subject and today's answer is correct: `IsSame`, `IsPartner`, `IsEqual` and `IsNotEqual` compare handle values and members with no dereference, `NbChildren` is guarded by OCCT itself, and `OCCTShapeIsEmpty` is the documented reader for `Shape.nullified`. Thirteen exceptions mean two accessors and a per-site choice between them, which is the per-consumer census again with a larger diff. CLAUDE.md's #1026 paragraph said a null shape is unsafe only on the ten `TopoDS_Shape` members. That is now half the rule, and the half it was missing is the one that crashed.
`wires` is a `const OCCTWireRef*`, so the mechanical pass that rewrote every `!x` opener produced `occtShapeIsPresent(wires)`, which does not compile and would have tested the wrong thing if it did. The array pointer test belongs at the top and the element test belongs in the loop, next to the `BRepBuilderAPI_MakeWire::Add` that dereferences it. Caught by the compiler rather than by review. Swept the other three array-taking wrapper parameters for the same mistake: none.
…note `occtBRepFeatCylindricalHole` is one of the two functions whose pointer-test refusal differs from its catch fallback, so its entry point is worth measuring rather than leaving named and unchecked. `BRepFeat_MakeCylindricalHole::Init` returns for a null shape and `Perform` raises a catchable `Standard_Failure` the existing `catch (...)` absorbs, so the pointer-only test there is correct and the divergence is a code, not a crash. 63 probes now: 17 uncatchable, 5 catchable, 41 return.
Two naming decisions #1026 left rather than acted on, because both are source-visible rather than crash fixes. isEmptyShape is TopoDS_Shape::IsNull() and nothing else, but the name reads as "has no sub-shapes", which is a different question and one the adjacent `emptied` answers yes to. Measured through the real API: box.emptied!.faces().count 0 no content box.emptied!.isNull false still a Solid box.nullified!.isNull true So two adjacent public members used "empty" for contradictory things. Renamed to isNull, which is what it measures, with isEmptyShape kept as a deprecated alias so the rename is source-compatible until the next major. `emptied`'s own doc now names the distinction from its side. nullified is deprecated in favour of emptied. Nullify() is how OCCT clears a local variable rather than a value to hand around: the result has no topological type, and until #1026 it crashed nine other public properties. The issue's grep across all 25 ecosystem repos found one hit, a vendored checkout of this repo's own test suite, so there is no caller to break. The trap test was run once against a broken subject: with the bridge's IsNull() swapped for NbChildren() == 0, which is the reading the old name invited, it fails on `(emptied.isNull -> true) == false` and passes once restored. Every in-repo caller moved to the new name. count-operations derives 4,351, README and API_REFERENCE rewritten by --fix rather than by hand. The reference entry is renamed and records why. Gates 8/8, style manifest clean, swiftlint 0, full swift test 5,783 in 1,490 suites green. Closes #1034
`ShapeFix_Shape::Perform` is in the table and the gate cannot reach `OCCTShapeFixerCreate`'s route to it, which goes through a Handle stored on a bridge-owned struct. That site is guarded by hand; the comment now says so rather than leaving the table looking more complete than it is. Also reorders a #1026 sentence the generalisation had left stranded.
…divergence 33 public APIs across eight pages now say what they answer for a null shape instead of what they used to do to the process. Every refusal value was read from the bridge source rather than inferred. Three guarded bridge functions get no entry because they have no Swift caller, and roughly thirty more are guarded on the `OCCTEdgeRef`/`OCCTFaceRef`/ `OCCTWireRef` wrappers, which no public producer can hand back carrying a null topology: `Edge(_:)`, `Face(_:)` and `Wire(_:)` all refuse one first. Those are contract pins, not reachable behaviour changes, and claiming otherwise in the docs would be inventing a change. `OCCTWireJoin` skips a null element where `OCCTShapeCreateCompound` refuses the whole call. That divergence is pre-existing, is this function's own contract for a null pointer, and is now named in the source rather than left for the next reader to find.
…or/385-unwrap-guard
fix(#1034): isEmptyShape becomes isNull, and nullified is deprecated
Guard the null shape where the consumer dereferences it, measured rather than everywhere (#1035)
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
1 participant
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.
Pass 4a (#385), the features lane of the segmented duplication audit (#377). Twenty-four PRs, integrated on this branch and verified as a whole rather than one at a time.
The closing summary is on #385: scope claimed and handed off, the twelve candidates that proved not real, what was deliberately not fixed, and the corrections to this pass's own record. Read that first; this body is the shape of the diff.
Verification of the integrated branch
swift testderive-gdt-enumscheck-style-manifest.py --base origin/maincount-operations.pykernel-integration.ymlEvery merge was verified on this branch after landing, not just on its own PR. The count gate fired correctly on four rebases and was re-derived each time rather than hand-edited.
What this is
Duplication was the entry point, not the boundary, as #377 intends. The pass produced more behaviour fixes than deduplications, in three recurring classes:
Values that were never computed, returned as measurements.
Surface.plateErrorsreturned uninitialised memory (1.94e-313,-3.11e+231,-nan, measured through the real bridge).DraftInfo's six public members queried a default-constructed throwaway or echoed their input; the file is deleted. A GD&T range dimension read back as a plain dimension with zero tolerance, publishing OCCT's "not applicable" zeroes as if measured.Parameters that did nothing. Thirteen bridge functions declared a parameter they never read.
Drawing's.perspectivewas accepted and discarded, so every perspective request silently returned an orthographic projection.maxIterationswas passed as a plate resolution order, so an out-of-range value returned an undeformed surface reporting success.A null
TopoDS_Shapereaching the kernel. Public API to public API:Shape.wireFromEdges([box.nullified!])was an uncatchable SIGSEGV. Three separate censuses each found a different subset before the true figure settled at 46 functions across seven files.Breaking changes
Nine public symbols removed from the GD&T read surface (one family survives),
DraftInfodeleted,Surface.plateErrorsdeleted,Drawing.ProjectionTypeno longerUInt32-backed,SAWireAnalysis.checkOuterBoundtakes the wire its siblings take and its polarity is inverted, and two mutators returnfalsewhere they previously returnedtruefor inputs whose documented postcondition they never met.Each is named with its migration in the
## UnreleasedCHANGELOG entries. Persemver-at-release.md,docs/SEMVER.mdis untouched: the break table is assembled once, at release, from the## SemVer impactstatements in the merged PR bodies.Two things a reviewer should know
Two PRs carry large reformats that are not scope creep. #1015 and #1016 had to bring ten files fully clean and off the style manifests, because I told that agent the files it needed were already clean and they were not. The ratchet then required it in the same PR.
Three new detectors ship under
Scripts/repro/, and none is a gate. Each carries a measured caveat, including its own first wrong answer.detect-hardcoded-arguments.pyis roughly two in three false on the one sample anyone has adjudicated. They are screening tools for #1001 to decide on, not verdicts.Follow-ups filed, deliberately not in this branch
#1009's sibling layouts, #1010, #1021, #1030, and #1001, the re-sweep of the closed passes. Plus two upstream OCCT PRs, #1481 and #1483.
CHANGELOG entry
The
## Unreleasedsection carries all twenty-one entries, transcribed from their PR bodies at merge.check-changelog-transcription.pyaccounts for every merge on this branch; the two it still reports MISSING (d74c7d78,2ea6e095) are Pass 3's and predate this work.SemVer impact
MAJOR. The removals and the two return-value corrections above are source-breaking. Detail per change is in each merged PR's own
## SemVer impact, which is whatdocs/SEMVER.mdis assembled from at release.Closing keywords
One per issue, deliberately.
Closes #1, #2closes only the first, and every fix PR in this passmerged into this branch rather than into
main, so none of their own keywords has fired. Onlythree of the merged commits carry a keyword at all; the rest say
Refs. Without this list,twenty-four fixed issues would stay open after this merges.
Closes #972
Closes #974
Closes #975
Closes #988
Closes #989
Closes #990
Closes #991
Closes #994
Closes #995
Closes #996
Closes #999
Closes #1000
Closes #1004
Closes #1008
Closes #1009
Closes #1017
Closes #1018
Closes #1019
Closes #1020
Closes #1022
Closes #1026
Closes #1036
Closes #1037
Closes #1038
Closes #1034
Closes #1035
Closes #385
Deliberately not closed, each a filed follow-up rather than an omission: #811 (refman coverage
for this lane, scheduled after the pass), #1001 (re-sweep the closed passes), #1010, #1021 (the
XCAFDoc_DimTolToolrow is adjudicated in #1028; seven class rows remain), #1030, #1034, #1035.