Map algo extraction #19
Conversation
…S#211 Refactor TCollection to use NCollection_Primes and remove obsolete TCollection.cxx New prime declaration now will be more clear to use.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request introduces a comprehensive refactoring of map and collection operations in the Open CASCADE Technology (OCCT) library. The primary focus is on introducing a new Changes
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/TCollection/TCollection.hxx (1)
40-43: Leverage inline for minimal overhead
Delegating toNCollection_Primes::NextPrimeForMapis appropriate for ensuring a single source of prime logic. Since this function is trivial, consider marking itinlineat callsite to minimize call overhead and maintain performance.src/NCollection/NCollection_MapAlgo.hxx (1)
1-286: Overall architecture feedback
The new file centralizes common set-like operations (union, intersection, difference, etc.) forNCollection_Map. This separation of algorithmic logic into a dedicated namespace is a good step toward SRP (Single Responsibility Principle) and improves maintainability. Below are specific observations:
Self-check references
Many functions handle the scenario where the result map is the same as one of the operand maps. The approach of using a temporary copy is well-structured to avoid corruption. This is correct and robust.Avoid repeated iteration
In functions such asUnion,Intersection, etc., the addition of elements from both maps can be further optimized by combining iterators or short-circuiting if a map is empty. The current approach is clear, but for large maps, consider whether repeated checks or merges can be streamlined.Potential concurrency
These algorithms are not obviously thread-safe and are designed for single-threaded usage. If concurrency becomes a requirement, additional synchronization or concurrency-safe patterns may be necessary.Iterator reusability
Repeatedly constructingtypename MapType::Iteratorinside loops is typical but might be turned into helper methods for clarity; however, this is more of a style choice.Unit test coverage
Ensure each operation (Union, Unite, Difference, Differ, etc.) has test coverage that checks edge cases such as empty maps, same map references, or large map performance.src/BRepOffset/BRepOffset_MakeOffset_1.cxx (2)
5889-5890: Consider combining intersection checks for performance.Here, two consecutive calls to
HasIntersection()might lead to duplicated set-iteration logic. If performance is critical and both sets are large, consider unifying these checks in a single pass or caching partial results.
5917-5918: Avoid duplicating intersection logic.This block is similar to lines 5889-5890 but for the “J” iteration. If possible, consider merging or extracting the logic to avoid repetition and reduce maintenance overhead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
src/BRepMesh/BRepMesh_FaceChecker.cxx(1 hunks)src/BRepMesh/BRepMesh_ModelHealer.cxx(1 hunks)src/BRepMesh/BRepMesh_NURBSRangeSplitter.cxx(1 hunks)src/BRepOffset/BRepOffset_MakeOffset_1.cxx(3 hunks)src/NCollection/FILES(1 hunks)src/NCollection/NCollection_BaseMap.cxx(4 hunks)src/NCollection/NCollection_BaseMap.hxx(5 hunks)src/NCollection/NCollection_BasePointerVector.cxx(0 hunks)src/NCollection/NCollection_Map.hxx(2 hunks)src/NCollection/NCollection_MapAlgo.hxx(1 hunks)src/NCollection/NCollection_Primes.cxx(1 hunks)src/NCollection/NCollection_Primes.hxx(1 hunks)src/QABugs/QABugs_19.cxx(4 hunks)src/QANCollection/QANCollection_Test.cxx(1 hunks)src/ShapeUpgrade/ShapeUpgrade_UnifySameDomain.cxx(1 hunks)src/TColStd/TColStd_PackedMapOfInteger.cxx(2 hunks)src/TCollection/FILES(0 hunks)src/TCollection/TCollection.cxx(0 hunks)src/TCollection/TCollection.hxx(2 hunks)src/XCAFDoc/XCAFDoc_Editor.cxx(1 hunks)
💤 Files with no reviewable changes (3)
- src/TCollection/TCollection.cxx
- src/NCollection/NCollection_BasePointerVector.cxx
- src/TCollection/FILES
✅ Files skipped from review due to trivial changes (1)
- src/ShapeUpgrade/ShapeUpgrade_UnifySameDomain.cxx
🔇 Additional comments (42)
src/NCollection/NCollection_BaseMap.hxx (6)
52-54: Use of nullptr instead of NULL is a good modernization step.
Switching to nullptr offers better type safety and clarity in C++11 and later standards.
61-61: Consistent nullptr replacement
Updating all pointer initializations to nullptr ensures consistency and avoids ambiguity.
81-81: Clear pointer re-initialization
Assigning myNode to nullptr clarifies reset behavior and prevents dangling pointers.
91-91: Maintaining a consistent reset
Setting myNode to nullptr eliminates any possibility of referencing stale data after reset.
104-104: Improved pointer check
Using myNode != nullptr is preferable over NULL for pointer comparisons in modern C++.
164-165: Safe initialization of class members
Initializing myData1 and myData2 with nullptr adheres to modern coding conventions and prevents uninitialized pointer usage.
src/TCollection/TCollection.hxx (2)
20-20: Check dependency usage
Including <NCollection_Primes.hxx> here is logical for enabling immediate usage of the NCollection_Primes functionalities. Ensure that this direct include does not create circular dependencies, especially if TCollection is widely included throughout the codebase.
28-28: Consistent deprecation notice
Marking TCollection as deprecated in favor of NCollection_Primes is coherent. Verify whether consumers of TCollection have been notified to migrate and ensure no new references to this class are introduced.
src/NCollection/NCollection_Map.hxx (10)
19-19: Centralized algorithmic includes
Including "NCollection_MapAlgo.hxx" at the start is a straightforward way to delegate set-like operations. Confirm that no cyclical references arise from these includes.
391-392: Check advanced notice for removal
Deprecating Union(const NCollection_Map&, const NCollection_Map&) is consistent with the new approach. Ensure any code that still relies on this function is flagged for update before removal in 7.9.
401-402: Deprecation for Unite
Unite(...) delegating to NCollection_MapAlgo::Unite is fine. Confirm that performance overhead is similar or improved, given the new indirect call.
408-409: HasIntersection
Replacing the ad-hoc HasIntersection logic with NCollection_MapAlgo::HasIntersection unifies the approach. Good step to ensure consistent behavior across the codebase.
418-419: Intersection deprecation
Moving intersection logic into NCollection_MapAlgo is consistent with the modularization goal. No immediate concerns here.
428-429: Intersect
Same as above—delegation is standard. Carefully monitor if there's any overhead from passing large maps repeatedly.
438-439: Subtraction
Deprecated in favor of NCollection_MapAlgo::Subtraction. The code path is straightforward.
449-450: Subtract
Internal logic is replaced by the new utility. This keeps the class lean.
458-459: Difference
Symmetric difference is also relegated to the new namespace. Confirm tests cover tricky edge cases (e.g., disjoint sets vs. identical sets).
471-471: Differ
Concludes the pattern of set operations. The deprecation notice and call to NCollection_MapAlgo::Differ looks consistent.
src/QANCollection/QANCollection_Test.cxx (1)
404-405: Enhanced read clarity
Using NCollection_MapAlgo::HasIntersection(aM1, aM2) improves readability and consistency throughout the codebase. The combined condition is correct; just ensure the test logic covers all intersection states accurately.
src/NCollection/NCollection_Primes.hxx (4)
1-13: LGTM: License header is correctly formatted and complete
The license header follows the standard OCCT format and includes all necessary clauses.
14-18: LGTM: Header guards and includes are properly defined
The header guards follow the OCCT naming convention and the required header is included.
19-31: Well-documented namespace purpose and usage
The documentation clearly explains the purpose of the namespace and its usage for map bucket sizing.
32-35: LGTM: Clean and minimal API design
The namespace provides a single focused method with clear purpose and proper export macro.
src/NCollection/NCollection_Primes.cxx (2)
21-37: Well-chosen prime numbers with good documentation
The prime numbers are well-selected with approximately 2x growth factor. The documentation explains the memory overhead tradeoff and references the performance improvement issue OCC13189.
39-47: Efficient implementation with proper error handling
The implementation:
- Uses std::lower_bound for efficient lookup
- Properly handles out-of-range cases with descriptive error message
- Returns the next prime number greater than or equal to the input
src/NCollection/NCollection_BaseMap.cxx (5)
19-19: LGTM: Updated include to use new NCollection_Primes header
Correctly replaces the old TCollection.hxx include with the new NCollection_Primes.hxx.
49-49: Good: Using modern nullptr instead of NULL
Updated to use C++11's nullptr which is type-safe compared to NULL macro.
103-103: LGTM: Size reset moved inside non-empty check
The mySize reset is now correctly placed inside the !IsEmpty() check.
111-111: Good: Using nullptr for pointer reset
Consistently using nullptr for pointer nulling.
176-176: LGTM: Updated to use new prime number utility
Correctly forwards to NCollection_Primes::NextPrimeForMap.
src/NCollection/FILES (1)
49-49: LGTM: New files properly added to FILES list
The new NCollection_MapAlgo.hxx, NCollection_Primes.cxx and NCollection_Primes.hxx files are correctly added to the FILES list in alphabetical order.
Also applies to: 53-54
src/BRepMesh/BRepMesh_FaceChecker.cxx (1)
312-312: Use of NCollection_MapAlgo::Unite for merging intersecting edges
This refactoring properly delegates the union operation to a dedicated utility function, aligning with the broader codebase effort to standardize map operations. The change is correct and maintains clarity.
src/BRepMesh/BRepMesh_ModelHealer.cxx (1)
238-238: Switching to NCollection_MapAlgo::Unite for edge aggregation
Replacing the direct map method call with NCollection_MapAlgo::Unite ensures consistency across the codebase. There appear to be no correctness or performance issues in this usage.
src/BRepMesh/BRepMesh_NURBSRangeSplitter.cxx (1)
364-365: Param sets subtraction using NCollection_MapAlgo::Subtract
Subtracting fixed parameters from removable ones clarifies parameter filtering and aligns with the standardized approach for map operations. This looks clean and correct, with no evident pitfalls.
src/TColStd/TColStd_PackedMapOfInteger.cxx (1)
156-156: Refactor to use NCollection_Primes::NextPrimeForMap
Switching from TCollection primes to NCollection_Primes unifies prime-based resizing logic within the NCollection scope. This is consistent with the new architecture and maintains correct bucket size calculations.
src/XCAFDoc/XCAFDoc_Editor.cxx (1)
675-675: Use the new NCollection_MapAlgo::Unite approach consistently
This transition to NCollection_MapAlgo::Unite() for merging map sets ensures consistent usage across the codebase. Verify that all union operations now rely on NCollection_MapAlgo to maintain uniformity.
src/QABugs/QABugs_19.cxx (5)
1432-1432: Refactor for consistent usage of map operations
Switching to NCollection_MapAlgo::Union() is in line with the new standardized approach. Make sure to confirm that the overhead of repeated calls is acceptable given performance considerations.
1441-1441: Double-check intersection side effects
Using NCollection_MapAlgo::Intersection() is effective. Ensure that the input maps remain unchanged after the operation.
1452-1452: Standard naming alignment
Adopting NCollection_MapAlgo::Subtraction() aligns with the API's set operation naming convention.
1460-1460: Validate the subtraction order
In NCollection_MapAlgo::Subtraction(aMapSubsRL, aMapRight, aMapLeft), confirm the order of arguments accurately produces the intended result.
1469-1469: Check difference correctness
Applying NCollection_MapAlgo::Difference() merges two subtractions internally. Confirm it meets the expected logic for disjoint segments.
src/BRepOffset/BRepOffset_MakeOffset_1.cxx (1)
3365-3365: Verify pointer validity before usage.
The code calls NCollection_MapAlgo::HasIntersection(*pMVInverted, *pMVAll1) unconditionally. Please ensure pMVInverted and pMVAll1 are always non-null to avoid a possible runtime crash in edge cases.
Run the following script to check where these pointers are assigned or validated:
✅ Verification successful
Pointer validity is properly checked and ensured
The code analysis shows that both pointers are properly initialized and checked:
pMVInvertedandpMVAllare first obtained viaChangeSeek(), then checked for null. If null, they are initialized usingBound().- Similarly,
pMVInverted1andpMVAll1follow the same pattern - obtained viaChangeSeek(), checked for null, and initialized if needed.
The code structure ensures that both pointers are valid before the HasIntersection() call:
TopTools_MapOfShape* pMVInverted1 = theDMCBVAll.ChangeSeek (aCB1);
if (!pMVInverted1) {
pMVInverted1 = theDMCBVInverted.Bound (aCB1, TopTools_MapOfShape());
pMVAll1 = theDMCBVAll.Bound (aCB1, TopTools_MapOfShape());
GetVerticesOnEdges (aCB1, myInvertedEdges, *pMVInverted1, *pMVAll1);
}🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Scans the codebase for references to pMVInverted and pMVAll1, including assignments and checks.
rg "pMVInverted" -A 5
rg "pMVAll1" -A 5
Length of output: 3401
dpasukhi
force-pushed
the
map_algo_extraction
branch
4 times, most recently
from
02fd9dc to
71a448c
Compare
…de-SAS#213 Updated multiple classes to utilize the new NCollection_MapAlgo for union, intersection, and other set operations, improving code structure and maintainability. Deprecated older methods in NCollection_Map in favor of the new algorithmic approaches.
dpasukhi
force-pushed
the
map_algo_extraction
branch
from
71a448c to
727f8fa
Compare
Summary by CodeRabbit
Release Notes
New Features
NCollectionmodule.NCollection_Primesclass.Bug Fixes
Refactor
NCollection_MapAlgofor union, intersection, and other set operations, improving code structure and maintainability.NCollection_Mapin favor of the new algorithmic approaches.Chores