Conversation
This commit eliminates double-lookup anti-patterns where code checks if a key exists (IsBound/Contains/FindIndex) and then performs a separate lookup to access or insert the value. These patterns perform two hash lookups when one suffices. New API Methods Added: - NCollection_DataMap::TryBound(key, value) - binds only if key not present, returns pointer to item (preserves existing values, unlike Bound) - NCollection_DataMap::Seek(key) - returns const pointer or nullptr - NCollection_DataMap::ChangeSeek(key) - returns non-const pointer or nullptr - NCollection_IndexedDataMap::Bound(key, value) - returns pointer to item (new or existing), does not overwrite existing values - NCollection_IndexedDataMap::TryBound(key, value) - alias for Bound with clearer semantics for non-overwriting behavior - NCollection_IndexedMap::Added(key) - returns reference to key (new or existing) Pattern Transformations Applied: - IsBound + Find -> Seek (returns pointer, null-safe) - IsBound + ChangeFind -> ChangeSeek (returns mutable pointer) - IsBound + Bind (preserve existing) -> TryBound - Contains + Add -> Add (for Map/IndexedMap) - FindIndex==0 + Add -> Add (IndexedMap returns existing index) - Multiple lookups consolidated to single Bound/Seek operations Files Modified: 76 files across all major modules - FoundationClasses: NCollection headers, Storage, Resource, Plugin, OSD - ModelingData: BRepTools (History, Modifier, Quilt, ReShape, etc.) - ModelingAlgorithms: TKBO, TKBool, TKFeat, TKMesh, TKOffset, TKShHealing, TKTopAlgo, TKGeomAlgo, TKExpress - ApplicationFramework: TNaming, BinMDF, StdStorage, TPrsStd - DataExchange: TKXCAF, TKXSBase, TKDESTEP, TKDEIGES, TKDEVRML, TKRWMesh - Visualization: OpenGl_Context, AIS_ColoredShape, AIS_InteractiveContext - Draw: DNaming, MeshTest Unit Tests Added: - NCollection_DataMap_Test.cxx: TryBound_NoOverwrite test - NCollection_IndexedDataMap_Test.cxx: Bound* tests (5 new tests) - NCollection_IndexedMap_Test.cxx: Added* tests (6 new tests) Semantic Notes: - TryBound preserves existing values (non-overwriting) - Bound overwrites existing values (for DataMap) or preserves (for IndexedDataMap) - For IndexedMap, Add() returns int (index), use Add()==Extent() to detect new
…and GetFreeWires methods for improved error handling
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThis pull request refactors map access patterns across the OCCT codebase. The changes systematically replace two-step lookup patterns (IsBound/Find or Contains/Add) with single-step pointer-based alternatives (Seek, TryBound, Bound, Added). New methods are introduced in NCollection_DataMap, NCollection_IndexedDataMap, and NCollection_IndexedMap to support these patterns. Affected code spans foundation classes, data exchange modules, modeling algorithms, and visualization systems. The modifications maintain functional behavior while altering control flow to rely on pointer validity checks instead of separate bound checks and to use method return values for conditional logic. 🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ModelingAlgorithms/TKOffset/BRepOffset/BRepOffset_MakeOffset_1.cxx (1)
3999-4006: Line 4005 usesBoundwhich overwrites existing entries; useTryBoundinstead to preserve accumulated pairs across multiplebuildPairscalls.The
Boundmethod explicitly "overrides value previously bound to the key" (per NCollection_DataMap documentation). SincebuildPairsis called multiple times on the same*pFInterMap(at lines 4128 and 4180 within nested loops), the unconditionalBoundcall in the first loop overwrites previously accumulated pair maps with empty ones on each iteration, losing data. ReplaceBoundwithTryBoundto preserve existing entries and only create maps for newly encountered shapes.
🤖 Fix all issues with AI agents
In `@src/DataExchange/TKXSBase/Interface/Interface_MSG.cxx`:
- Around line 211-212: The new call to thedic.Bind(key, str) overwrites existing
entries because NCollection_DataMap::Bind replaces the value and returns false;
to restore the original "first-wins" semantics, check thedic.IsBound(key) before
calling thedic.Bind and only call Bind when IsBound returns false, otherwise use
the existing value from thedic (not str) to populate thedup (preserve the
original value) — update the code paths around thedic.Bind, thedup handling, and
any logic that assumed the original value to reference the existing value via
thedic.Lookup/Value accessors rather than the incoming str.
In `@src/ModelingAlgorithms/TKMesh/BRepMesh/BRepMesh_ModelPreProcessor.cxx`:
- Around line 326-343: The loop nullifies edges using the triangulation returned
by BRep_Tool::Triangulation(aDFace->GetFace(), aLoc) but does not check for a
null triangulation; update the logic inside the wire/edge loops so that for each
aTmpDEdge you call BRepMesh_ShapeTool::NullifyEdge(aTmpDEdge->GetEdge(),
aTriangulation, aLoc) only when aTriangulation is non-null, otherwise call the
overload BRepMesh_ShapeTool::NullifyEdge(aTmpDEdge->GetEdge()) (i.e. choose the
variant based on the result of BRep_Tool::Triangulation) and keep the existing
BRepMesh_ShapeTool::NullifyFace(aDFace->GetFace()) afterwards.
🧹 Nitpick comments (7)
src/ModelingAlgorithms/TKFeat/BRepFeat/BRepFeat_Builder.cxx (1)
540-545: Consider usingTryBoundto simplify the get-or-create pattern.Since this PR introduces
TryBoundwhich preserves existing values and returns a pointer to the (possibly newly created) entry, this block could be consolidated into a single call.♻️ Proposed simplification
- NCollection_List<TopoDS_Shape>* pLOr = myOrigins.ChangeSeek(aSx); - if (!pLOr) - { - pLOr = myOrigins.Bound(aSx, NCollection_List<TopoDS_Shape>()); - } - pLOr->Append(aF); + myOrigins.TryBound(aSx, NCollection_List<TopoDS_Shape>())->Append(aF);src/DataExchange/TKXSBase/IFSelect/IFSelect_SignatureList.cxx (1)
69-81: Consider applying the sameBoundpattern tothediclisthandling.This block still uses the Contains/FindFromKey double-lookup pattern that the PR is aiming to eliminate. While the initialization logic makes it slightly less elegant than the
thedicountcase, it could still benefit from a single-lookup approach:♻️ Suggested refactor
if (thelistat) { - occ::handle<NCollection_HSequence<occ::handle<Standard_Transient>>> alist; - if (thediclist.Contains(sign)) - alist = occ::down_cast<NCollection_HSequence<occ::handle<Standard_Transient>>>( - thediclist.FindFromKey(sign)); - else + occ::handle<Standard_Transient>* pList = thediclist.Bound(sign, occ::handle<Standard_Transient>()); + occ::handle<NCollection_HSequence<occ::handle<Standard_Transient>>> alist; + if (pList->IsNull()) { alist = new NCollection_HSequence<occ::handle<Standard_Transient>>(); - thediclist.Add(sign, alist); + *pList = alist; } + else + { + alist = occ::down_cast<NCollection_HSequence<occ::handle<Standard_Transient>>>(*pList); + } alist->Append(ent); }This converts two lookups (Contains + FindFromKey) into one, consistent with the pattern applied to
thedicounton line 67.src/ModelingAlgorithms/TKShHealing/ShapeAnalysis/ShapeAnalysis_Shell.cxx (1)
92-95: PreferNCollection_IndexedMap::Added()over extent comparisons.Using
Add()+ priorExtent()(Line 92–95, Line 102–105) relies on Add’s return semantics. Since the PR introducesNCollection_IndexedMap::Added(), using it would make intent explicit and reduce coupling to Add()’s behavior.Also applies to: 102-105
src/FoundationClasses/TKernel/OSD/OSD_PerfMeter.cxx (2)
367-375: Consider simplifying now thatStopwatchStorage::Printhandles missing stopwatches.With the guard removed from
StopwatchStorage::Print, this existence check is redundant and causes a double lookup (GetStopwatch doesChangeSeek, then Print→print doesSeek).♻️ Suggested simplification
TCollection_AsciiString OSD_PerfMeter::Print() const { - Stopwatch* aStopwatch = StopwatchStorage::Instance().GetStopwatch(myMeterName); - if (aStopwatch != nullptr) - { - return StopwatchStorage::Instance().Print(myMeterName); - } - return ""; + return StopwatchStorage::Instance().Print(myMeterName); }
319-326: Double-lookup pattern remains inInit.This uses
HasStopwatch+CreateStopwatchto avoid overwriting existing stopwatches—exactly the pattern thatTryBoundwas introduced for in this PR. Could be consolidated into a single lookup.♻️ Potential approach using TryBound
Either add a new method to
StopwatchStorage:Stopwatch& GetOrCreateStopwatch(const TCollection_AsciiString& theName) { return *myStopwatches.TryBound(theName, Stopwatch()); }Or simplify
Initto:void OSD_PerfMeter::Init(const TCollection_AsciiString& theMeterName) { myMeterName = theMeterName; - if (!StopwatchStorage::Instance().HasStopwatch(myMeterName)) - { - StopwatchStorage::Instance().CreateStopwatch(myMeterName); - } + StopwatchStorage::Instance().GetOrCreateStopwatch(myMeterName); }src/ModelingAlgorithms/TKBool/TopOpeBRepBuild/TopOpeBRepBuild_Builder1.cxx (1)
1853-1856: Consider usingNCollection_IndexedMap::Added()for clarity.The Extent+Add pattern works but is indirect and relies on Add’s index semantics. If
myProcessedPartsON2dis anNCollection_IndexedMap, the newAdded()API reads clearer and avoids the extra Extent call.♻️ Possible refactor (apply to all three blocks)
-const int aPrevExtent = myProcessedPartsON2d.Extent(); -if (myProcessedPartsON2d.Add(aPieceObj) > aPrevExtent) +if (myProcessedPartsON2d.Added(aPieceObj)) { IsSame2d(aSeq, aListOfPiecesOut2d); }Also applies to: 1861-1864, 1930-1933
src/ModelingAlgorithms/TKBool/BRepFill/BRepFill_Evolved.cxx (1)
1424-1434: Consider caching the sub-map to avoid duplicate ChangeFind.
This keeps the refactor aligned with the “single lookup” objective.♻️ Suggested refactor
- NCollection_List<TopoDS_Shape>* pListV2 = - myMap.ChangeFind(it.Key()).TryBound(V2, NCollection_List<TopoDS_Shape>()); - NCollection_List<TopoDS_Shape>* pListE = - myMap.ChangeFind(it.Key()).TryBound(E, NCollection_List<TopoDS_Shape>()); + auto& aSubMap = myMap.ChangeFind(it.Key()); + NCollection_List<TopoDS_Shape>* pListV2 = + aSubMap.TryBound(V2, NCollection_List<TopoDS_Shape>()); + NCollection_List<TopoDS_Shape>* pListE = + aSubMap.TryBound(E, NCollection_List<TopoDS_Shape>());
| if (thedic.Bind(key, str)) | ||
| return; // newly added, skip the rest |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for NCollection_DataMap Bind method definition
rg -n "bool.*Bind\s*\(" --type-add 'hxx:*.hxx' --type hxx -A 15 | head -100Repository: dpasukhi/OCCT
Length of output: 9734
🏁 Script executed:
# Also search more broadly for NCollection_DataMap definition
rg -l "NCollection_DataMap" | head -10Repository: dpasukhi/OCCT
Length of output: 736
🏁 Script executed:
# Check the include files to locate the template definition
find . -name "*NCollection_DataMap*" -type fRepository: dpasukhi/OCCT
Length of output: 192
Bind semantics confirm a critical semantic change.
The verification shows that NCollection_DataMap::Bind overwrites existing values when a key is already bound (line 282 of NCollection_DataMap.hxx: aNode->ChangeValue() = theItem;), then returns false. This means:
- Original behavior: First binding preserved; duplicate stored in
thedupwith original value intact - New behavior: Duplicate value overwrites the original in the map before the duplicate-handling code (lines 213-225) executes
This changes the semantics: the map now contains the new/duplicate value, not the original. If the duplicate-handling code expects the original value to still be present, this will cause incorrect behavior.
Use IsBound() to check existence first and preserve the "first-wins" semantics, or document that overwriting duplicates is now the intended behavior.
🤖 Prompt for AI Agents
In `@src/DataExchange/TKXSBase/Interface/Interface_MSG.cxx` around lines 211 -
212, The new call to thedic.Bind(key, str) overwrites existing entries because
NCollection_DataMap::Bind replaces the value and returns false; to restore the
original "first-wins" semantics, check thedic.IsBound(key) before calling
thedic.Bind and only call Bind when IsBound returns false, otherwise use the
existing value from thedic (not str) to populate thedup (preserve the original
value) — update the code paths around thedic.Bind, thedup handling, and any
logic that assumed the original value to reference the existing value via
thedic.Lookup/Value accessors rather than the incoming str.
| if (aUsedFaces.Add(aDFace.get()) && aDFace->IsSet(IMeshData_Outdated)) | ||
| { | ||
| aUsedFaces.Add(aDFace.get()); | ||
| if (aDFace->IsSet(IMeshData_Outdated)) | ||
| { | ||
| TopLoc_Location aLoc; | ||
| const occ::handle<Poly_Triangulation>& aTriangulation = | ||
| BRep_Tool::Triangulation(aDFace->GetFace(), aLoc); | ||
| TopLoc_Location aLoc; | ||
| const occ::handle<Poly_Triangulation>& aTriangulation = | ||
| BRep_Tool::Triangulation(aDFace->GetFace(), aLoc); | ||
|
|
||
| // Clean all edges of oudated face. | ||
| for (int aWireIt = 0; aWireIt < aDFace->WiresNb(); ++aWireIt) | ||
| // Clean all edges of oudated face. | ||
| for (int aWireIt = 0; aWireIt < aDFace->WiresNb(); ++aWireIt) | ||
| { | ||
| const IMeshData::IWireHandle& aDWire = aDFace->GetWire(aWireIt); | ||
| for (int aWireEdgeIt = 0; aWireEdgeIt < aDWire->EdgesNb(); ++aWireEdgeIt) | ||
| { | ||
| const IMeshData::IWireHandle& aDWire = aDFace->GetWire(aWireIt); | ||
| for (int aWireEdgeIt = 0; aWireEdgeIt < aDWire->EdgesNb(); ++aWireEdgeIt) | ||
| { | ||
| const IMeshData::IEdgeHandle aTmpDEdge = aDWire->GetEdge(aWireEdgeIt); | ||
| BRepMesh_ShapeTool::NullifyEdge(aTmpDEdge->GetEdge(), aTriangulation, aLoc); | ||
| } | ||
| const IMeshData::IEdgeHandle aTmpDEdge = aDWire->GetEdge(aWireEdgeIt); | ||
| BRepMesh_ShapeTool::NullifyEdge(aTmpDEdge->GetEdge(), aTriangulation, aLoc); | ||
| } | ||
|
|
||
| BRepMesh_ShapeTool::NullifyFace(aDFace->GetFace()); | ||
| } | ||
|
|
||
| BRepMesh_ShapeTool::NullifyFace(aDFace->GetFace()); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find BRepMesh_ShapeTool implementation files
fd -a "BRepMesh_ShapeTool" --extension cxx --extension hxx --extension hpp --extension cppRepository: dpasukhi/OCCT
Length of output: 218
🏁 Script executed:
# Check the header file for NullifyEdge declarations
cat -n /home/jailuser/git/src/ModelingAlgorithms/TKMesh/BRepMesh/BRepMesh_ShapeTool.hxx | head -200Repository: dpasukhi/OCCT
Length of output: 8402
🏁 Script executed:
# Search for NullifyEdge implementations in the .cxx file
grep -n "NullifyEdge" /home/jailuser/git/src/ModelingAlgorithms/TKMesh/BRepMesh/BRepMesh_ShapeTool.cxx -A 20Repository: dpasukhi/OCCT
Length of output: 1722
🏁 Script executed:
# Look at the context around line 326-343 in the ModelPreProcessor file
sed -n '315,355p' /home/jailuser/git/src/ModelingAlgorithms/TKMesh/BRepMesh/BRepMesh_ModelPreProcessor.cxxRepository: dpasukhi/OCCT
Length of output: 1262
🏁 Script executed:
# Search for other occurrences of BRep_Tool::Triangulation in the file to see if null-checks are done
grep -n "BRep_Tool::Triangulation" /home/jailuser/git/src/ModelingAlgorithms/TKMesh/BRepMesh/BRepMesh_ModelPreProcessor.cxx -B 2 -A 5Repository: dpasukhi/OCCT
Length of output: 1220
🏁 Script executed:
# Let's check the entire conditional block to understand the logic better
sed -n '320,350p' /home/jailuser/git/src/ModelingAlgorithms/TKMesh/BRepMesh/BRepMesh_ModelPreProcessor.cxxRepository: dpasukhi/OCCT
Length of output: 1116
Add null-check for triangulation before calling NullifyEdge with it.
BRep_Tool::Triangulation() can return null for outdated faces, but the code at line 338 passes it to NullifyEdge without checking. Use the overload without triangulation if null; otherwise use the variant with triangulation.
🛠️ Proposed fix
const occ::handle<Poly_Triangulation>& aTriangulation =
BRep_Tool::Triangulation(aDFace->GetFace(), aLoc);
// Clean all edges of oudated face.
for (int aWireIt = 0; aWireIt < aDFace->WiresNb(); ++aWireIt)
{
const IMeshData::IWireHandle& aDWire = aDFace->GetWire(aWireIt);
for (int aWireEdgeIt = 0; aWireEdgeIt < aDWire->EdgesNb(); ++aWireEdgeIt)
{
const IMeshData::IEdgeHandle aTmpDEdge = aDWire->GetEdge(aWireEdgeIt);
- BRepMesh_ShapeTool::NullifyEdge(aTmpDEdge->GetEdge(), aTriangulation, aLoc);
+ if (aTriangulation.IsNull())
+ {
+ BRepMesh_ShapeTool::NullifyEdge(aTmpDEdge->GetEdge(), aLoc);
+ }
+ else
+ {
+ BRepMesh_ShapeTool::NullifyEdge(aTmpDEdge->GetEdge(), aTriangulation, aLoc);
+ }
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (aUsedFaces.Add(aDFace.get()) && aDFace->IsSet(IMeshData_Outdated)) | |
| { | |
| aUsedFaces.Add(aDFace.get()); | |
| if (aDFace->IsSet(IMeshData_Outdated)) | |
| { | |
| TopLoc_Location aLoc; | |
| const occ::handle<Poly_Triangulation>& aTriangulation = | |
| BRep_Tool::Triangulation(aDFace->GetFace(), aLoc); | |
| TopLoc_Location aLoc; | |
| const occ::handle<Poly_Triangulation>& aTriangulation = | |
| BRep_Tool::Triangulation(aDFace->GetFace(), aLoc); | |
| // Clean all edges of oudated face. | |
| for (int aWireIt = 0; aWireIt < aDFace->WiresNb(); ++aWireIt) | |
| // Clean all edges of oudated face. | |
| for (int aWireIt = 0; aWireIt < aDFace->WiresNb(); ++aWireIt) | |
| { | |
| const IMeshData::IWireHandle& aDWire = aDFace->GetWire(aWireIt); | |
| for (int aWireEdgeIt = 0; aWireEdgeIt < aDWire->EdgesNb(); ++aWireEdgeIt) | |
| { | |
| const IMeshData::IWireHandle& aDWire = aDFace->GetWire(aWireIt); | |
| for (int aWireEdgeIt = 0; aWireEdgeIt < aDWire->EdgesNb(); ++aWireEdgeIt) | |
| { | |
| const IMeshData::IEdgeHandle aTmpDEdge = aDWire->GetEdge(aWireEdgeIt); | |
| BRepMesh_ShapeTool::NullifyEdge(aTmpDEdge->GetEdge(), aTriangulation, aLoc); | |
| } | |
| const IMeshData::IEdgeHandle aTmpDEdge = aDWire->GetEdge(aWireEdgeIt); | |
| BRepMesh_ShapeTool::NullifyEdge(aTmpDEdge->GetEdge(), aTriangulation, aLoc); | |
| } | |
| BRepMesh_ShapeTool::NullifyFace(aDFace->GetFace()); | |
| } | |
| BRepMesh_ShapeTool::NullifyFace(aDFace->GetFace()); | |
| if (aUsedFaces.Add(aDFace.get()) && aDFace->IsSet(IMeshData_Outdated)) | |
| { | |
| TopLoc_Location aLoc; | |
| const occ::handle<Poly_Triangulation>& aTriangulation = | |
| BRep_Tool::Triangulation(aDFace->GetFace(), aLoc); | |
| // Clean all edges of oudated face. | |
| for (int aWireIt = 0; aWireIt < aDFace->WiresNb(); ++aWireIt) | |
| { | |
| const IMeshData::IWireHandle& aDWire = aDFace->GetWire(aWireIt); | |
| for (int aWireEdgeIt = 0; aWireEdgeIt < aDWire->EdgesNb(); ++aWireEdgeIt) | |
| { | |
| const IMeshData::IEdgeHandle aTmpDEdge = aDWire->GetEdge(aWireEdgeIt); | |
| if (aTriangulation.IsNull()) | |
| { | |
| BRepMesh_ShapeTool::NullifyEdge(aTmpDEdge->GetEdge(), aLoc); | |
| } | |
| else | |
| { | |
| BRepMesh_ShapeTool::NullifyEdge(aTmpDEdge->GetEdge(), aTriangulation, aLoc); | |
| } | |
| } | |
| } | |
| BRepMesh_ShapeTool::NullifyFace(aDFace->GetFace()); |
🤖 Prompt for AI Agents
In `@src/ModelingAlgorithms/TKMesh/BRepMesh/BRepMesh_ModelPreProcessor.cxx` around
lines 326 - 343, The loop nullifies edges using the triangulation returned by
BRep_Tool::Triangulation(aDFace->GetFace(), aLoc) but does not check for a null
triangulation; update the logic inside the wire/edge loops so that for each
aTmpDEdge you call BRepMesh_ShapeTool::NullifyEdge(aTmpDEdge->GetEdge(),
aTriangulation, aLoc) only when aTriangulation is non-null, otherwise call the
overload BRepMesh_ShapeTool::NullifyEdge(aTmpDEdge->GetEdge()) (i.e. choose the
variant based on the result of BRep_Tool::Triangulation) and keep the existing
BRepMesh_ShapeTool::NullifyFace(aDFace->GetFace()) afterwards.
1 participant
No description provided.