Repository navigation
Conversation
… by the point sequence Both classes count their extrema with mySqDist.Length() and bound Points() against that count alone. The analytic branch for a parallel pair appends a distance and no point pair, so NbExt() reports 1 while the point sequences are empty, and Points(1, ...) reads an empty NCollection_Sequence. Bound Points() against the point sequence, as Extrema_ExtCC::Points does. Both classes keep the sequences in step with mySqDist in every other branch, so a non-parallel result is unchanged. Add GTests for the parallel pair and for a non-parallel pair of each class.
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.
Pre-Submission Checks
.github/CONTRIBUTING.md.Group - Summaryformat.Problem / Motivation
Extrema_ExtSSandExtrema_ExtCScount their extrema withmySqDist.Length()and boundPoints()against that count alone:The analytic branch for a parallel pair appends a distance and no point pair:
An equidistant family has no unique witness point, so there is nothing to append.
NbExt()then reports 1,Points(1, ...)passes its range test and reads an emptyNCollection_Sequence.Extrema_ExtCShas the same shape (myExtElCS.IsParallel(),myPOnC/myPOnS).In a release build
NCollection_Sequence::Valuehas its range check compiled out (No_Exception), so the read faults instead of raising: two parallel planes, or a line parallel to a plane, end the process with SIGSEGV.Extrema_ExtCC::Pointshas the same defect and is handled in #1445.Proposed Solution
Points()against the point sequence (myPOnS1.Length(),myPOnC.Length()), so a parallel pair raisesStandard_OutOfRangefor the point it does not have.NbExt()andSquareDistance()are unchanged and still report the distance of the parallel pair.mySqDistand the point sequences are appended together in every other branch, so the new bound is the same number and nothing changes there.Points()documentation of both classes.Extrema_ExtSS_TestandExtrema_ExtCS_Test: a parallel pair (distance kept,Points()raises) and a non-parallel pair (every extremum still has its points), so the tighter bound is shown not to refuse a result that has points.Extrema_ExtCC2dandExtrema_ExtElC2dboundPoints()against counters that move in step with their point containers, andExtrema_ExtElC,Extrema_ExtElCSandExtrema_ExtElSSappend no distances of their own, so these two classes are the remaining ones with the mismatch.Validation
Local build of current
IR(Release,BUILD_RELEASE_DISABLE_EXCEPTIONSon as CI builds it, macOS arm64, clang),OpenCascadeGTest:IRwithout the source changeExtrema_ExtSS_Test.ParallelPlanesHaveADistanceButNoPointsExtrema_ExtCS_Test.LineParallelToPlaneHasADistanceButNoPointsExtrema_ExtSS_Test.SeparatedSpheresStillReportTheirPointsExtrema_ExtCS_Test.LineAboveSphereStillReportsItsPointsThe two controls pass on both sides, so the first two rows are about the parallel branch and not about
Points()in general. With exceptions enabled the same call raisesStandard_OutOfRangefromNCollection_Sequenceinstead of faulting, so theEXPECT_THROWalso passes there; the change makes the exception come from the bound test ofPoints()itself.Neighbouring suites (
*Extrema*,GeomAPI*,*Distance*) with the change: 608 tests, 607 passed, 1 skipped by the test itself (ExtremaPC2d_GridEvaluatorTest.UniformParamsRejectInvalidSampleCount), none failed.Checks performed:
clang-format 18.1.8 with the repository
.clang-format, the license check and the include cleanup of the formatting job report no change on the touched files, and none contains a non-ASCII character.Review Notes
Independent of #1445, which changes only
Extrema_ExtCC. The two touch different files and do not conflict.