Foundation Classes - Refactor CSLib package and add GTests - #857
Conversation
dpasukhi
added
2. Enhancement
There was a problem hiding this comment.
Pull request overview
This PR comprehensively refactors the CSLib package to modern C++17 standards while fixing critical bugs and adding extensive test coverage. The refactoring modernizes the codebase by replacing OCCT typedefs with native C++ types (double, int, bool), applying consistent naming conventions (theParam, aLocal, myMember), and adding comprehensive Doxygen documentation. Critical bug fixes include correcting derivative calculations, initializing status variables, fixing sign change detection logic, and preventing division by zero. The PR also introduces 29 GTests covering normal computation, point-in-polygon classification, polynomial evaluation, and real surface scenarios.
Key changes:
- Modernized C++17 syntax: native types, std:: prefix, constexpr, auto, structured bindings
- Fixed 4 critical bugs in derivative calculations and edge detection
- Added 614 lines of comprehensive GTest coverage (29 tests across 5 test suites)
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
CSLib_Test.cxx |
New GTest file with 29 comprehensive tests for CSLib functionality |
FILES.cmake |
Added CSLib_Test.cxx to build system |
CSLib_NormalStatus.hxx |
Updated enum documentation with clearer descriptions |
CSLib_DerivativeStatus.hxx |
Updated enum documentation with clearer descriptions |
CSLib_NormalPolyDef.hxx |
Modernized signatures (int, double, bool) and added comprehensive documentation |
CSLib_NormalPolyDef.cxx |
Fixed derivative bug (missing multiplication), modernized code with std:: functions |
CSLib_Class2d.hxx |
Modernized signatures and added detailed documentation |
CSLib_Class2d.cxx |
Fixed division-by-zero bug, removed dead code, modernized implementation |
CSLib.hxx |
Modernized function signatures and added comprehensive documentation |
CSLib.cxx |
Fixed status initialization bug, fixed sign change logic bug, replaced manual sort with std::sort |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 9 comments.
Comments suppressed due to low confidence (1)
src/FoundationClasses/TKMath/GTests/CSLib_Test.cxx:1
- The variable types
Standard_Realshould be replaced withdoubleto follow the C++17 modernization guidelines.
// Copyright (c) 2025 OPEN CASCADE SAS
| void checkDirEqual(const gp_Dir& theDir1, | ||
| const gp_Dir& theDir2, | ||
| const Standard_Real theTolerance = 1e-10) |
There was a problem hiding this comment.
The parameter type Standard_Real should be replaced with double to follow the C++17 modernization guidelines. The function is a new helper in the test file and should use native types consistently.
| void checkVecEqual(const gp_Vec& theVec1, | ||
| const gp_Vec& theVec2, | ||
| const Standard_Real theTolerance = 1e-10) |
There was a problem hiding this comment.
The parameter type Standard_Real should be replaced with double to follow the C++17 modernization guidelines. The function is a new helper in the test file and should use native types consistently.
|
|
||
| CSLib_NormalPolyDef aPoly(0, aLambda); | ||
|
|
||
| Standard_Real aValue; |
There was a problem hiding this comment.
The variable type Standard_Real should be replaced with double to follow the C++17 modernization guidelines.
|
|
||
| CSLib_NormalPolyDef aPoly(1, aLambda); | ||
|
|
||
| Standard_Real aValue; |
There was a problem hiding this comment.
The variable type Standard_Real should be replaced with double to follow the C++17 modernization guidelines.
|
|
||
| CSLib_NormalPolyDef aPoly(2, aLambda); | ||
|
|
||
| Standard_Real aValue; |
There was a problem hiding this comment.
The variable type Standard_Real should be replaced with double to follow the C++17 modernization guidelines.
|
|
||
| CSLib_NormalPolyDef aPoly(2, aLambda); | ||
|
|
||
| Standard_Real aDeriv; |
There was a problem hiding this comment.
The variable type Standard_Real should be replaced with double to follow the C++17 modernization guidelines.
|
|
||
| CSLib_NormalPolyDef aPoly(2, aLambda); | ||
|
|
||
| Standard_Real aDeriv; |
There was a problem hiding this comment.
The variable type Standard_Real should be replaced with double to follow the C++17 modernization guidelines.
| Standard_Real aF1, aD1; | ||
| Standard_Real aF2, aD2; |
There was a problem hiding this comment.
The variable types Standard_Real should be replaced with double to follow the C++17 modernization guidelines.
| EXPECT_TRUE(aPoly.Value(aX + aH, aFPlus)); | ||
| EXPECT_TRUE(aPoly.Derivative(aX, aAnalyticDeriv)); | ||
|
|
||
| const Standard_Real aNumericDeriv = (aFPlus - aFMinus) / (2.0 * aH); |
There was a problem hiding this comment.
The variable type Standard_Real should be replaced with double to follow the C++17 modernization guidelines.
| i = 0; | ||
| Standard_Boolean definie = Standard_False; | ||
| while (i <= Order && !definie) | ||
| double r = aDerVec.Magnitude() / aVk0.Magnitude(); |
There was a problem hiding this comment.
Nondescript name that doesn't follow naming convention. Please, consider renaming.
…erformance - Updated CSLib_Class2d to enhance documentation and improve parameter types for constructors. - Refined point classification methods with clearer return types and improved comments. - Modified CSLib_DerivativeStatus to clarify the status of surface derivatives computation. - Enhanced CSLib_NormalPolyDef for better readability and efficiency in polynomial evaluations. - Updated CSLib_NormalStatus to provide clearer descriptions of normal computation statuses.
- Updated point classification methods to return clearer result types. - Enhanced input validation and array management for polygon vertices. - Improved documentation for methods and parameters. - Refactored internal methods for consistency and readability.
- Changed test cases in CSLib_Class2dTest to utilize actual coordinates instead of normalized coordinates for point classification. - Updated expected results to reflect the new testing approach, enhancing clarity and accuracy of the tests.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
src/FoundationClasses/TKMath/GTests/CSLib_Test.cxx:1
- Multiple variable declarations use
Standard_Realinstead ofdouble. For consistency with the modernization effort documented in the PR description, all test code should use native C++ types.
// Copyright (c) 2025 OPEN CASCADE SAS
|
|
||
| CSLib_NormalPolyDef aPoly(1, aLambda); | ||
|
|
||
| Standard_Real aValue; |
There was a problem hiding this comment.
Multiple variable declarations use Standard_Real instead of double. For consistency with the modernization effort documented in the PR description, all test code should use native C++ types.
|
|
||
| CSLib_NormalPolyDef aPoly(2, aLambda); | ||
|
|
||
| Standard_Real aDeriv; |
There was a problem hiding this comment.
Multiple variable declarations use Standard_Real instead of double. For consistency with the modernization effort documented in the PR description, all test code should use native C++ types.
|
|
||
| CSLib_NormalPolyDef aPoly(2, aLambda); | ||
|
|
||
| Standard_Real aDeriv; |
There was a problem hiding this comment.
Multiple variable declarations use Standard_Real instead of double. For consistency with the modernization effort documented in the PR description, all test code should use native C++ types.
| Standard_Real aF1, aD1; | ||
| Standard_Real aF2, aD2; |
There was a problem hiding this comment.
Multiple variable declarations use Standard_Real instead of double. For consistency with the modernization effort documented in the PR description, all test code should use native C++ types.
| Standard_Real aF1, aD1; | |
| Standard_Real aF2, aD2; | |
| double aF1, aD1; | |
| double aF2, aD2; |
| const Standard_Real aX = 0.8; // Away from singular points | ||
| const Standard_Real aH = 1e-6; | ||
|
|
||
| Standard_Real aFMinus, aFPlus, aAnalyticDeriv; | ||
|
|
||
| EXPECT_TRUE(aPoly.Value(aX - aH, aFMinus)); | ||
| EXPECT_TRUE(aPoly.Value(aX + aH, aFPlus)); | ||
| EXPECT_TRUE(aPoly.Derivative(aX, aAnalyticDeriv)); | ||
|
|
||
| const Standard_Real aNumericDeriv = (aFPlus - aFMinus) / (2.0 * aH); |
There was a problem hiding this comment.
Multiple variable declarations use Standard_Real instead of double. For consistency with the modernization effort documented in the PR description, all test code should use native C++ types.
| const Standard_Real aX = 0.8; // Away from singular points | |
| const Standard_Real aH = 1e-6; | |
| Standard_Real aFMinus, aFPlus, aAnalyticDeriv; | |
| EXPECT_TRUE(aPoly.Value(aX - aH, aFMinus)); | |
| EXPECT_TRUE(aPoly.Value(aX + aH, aFPlus)); | |
| EXPECT_TRUE(aPoly.Derivative(aX, aAnalyticDeriv)); | |
| const Standard_Real aNumericDeriv = (aFPlus - aFMinus) / (2.0 * aH); | |
| const double aX = 0.8; // Away from singular points | |
| const double aH = 1e-6; | |
| double aFMinus, aFPlus, aAnalyticDeriv; | |
| EXPECT_TRUE(aPoly.Value(aX - aH, aFMinus)); | |
| EXPECT_TRUE(aPoly.Value(aX + aH, aFPlus)); | |
| EXPECT_TRUE(aPoly.Derivative(aX, aAnalyticDeriv)); | |
| const double aNumericDeriv = (aFPlus - aFMinus) / (2.0 * aH); |
| { | ||
| aSol(aRootIdx) = aFindRoots.Value(aRootIdx); | ||
| } | ||
| std::sort(&aSol(1), &aSol(aNbSol) + 1); |
There was a problem hiding this comment.
The replacement of manual insertion sort with std::sort() is a good modernization, but there's a potential issue: the code takes the address of array elements using &aSol(1) which works for TColStd_Array1OfReal but is not the most robust approach. Consider using std::sort(aSol.begin() + 1, aSol.begin() + aNbSol + 1) if TColStd_Array1OfReal provides iterators, or document why direct pointer arithmetic is safe here.
| std::sort(&aSol(1), &aSol(aNbSol) + 1); | |
| std::sort(aSol.begin() + 1, aSol.begin() + aNbSol + 1); |
Refactored all CSLib package files to modern C++17 standards:
Bug fixes:
by myTABli(i) coefficient in derivative calculation
when no non-null derivative is found (now returns CSLib_Singular)
overwriting previous true value instead of preserving it
edges to prevent division by zero
Code cleanup:
Added comprehensive GTest coverage (29 tests):