Coding - Optimize double-lookup anti-patterns across codebase

[dpasukhi]

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

[Copilot]

Pull request overview

This PR optimizes the codebase by eliminating double-lookup anti-patterns across 76 files. The optimization replaces patterns where code checks for key existence and then performs a separate lookup with single-operation methods. New API methods (Seek, ChangeSeek, TryBound, Bound, Added) were added to NCollection containers to enable these optimizations, with comprehensive unit test coverage.

Changes:

• Added new API methods to NCollection_DataMap, NCollection_IndexedDataMap, and NCollection_IndexedMap for single-lookup operations
• Replaced double-lookup patterns (IsBound+Find, Contains+Add, FindIndex+Add) with single-operation equivalents across all major modules
• Added unit tests validating the new API methods preserve existing behavior and prevent value overwrites where appropriate

Reviewed changes

Copilot reviewed 76 out of 76 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
NCollection_DataMap.hxx	Added TryBound and Seek methods for non-overwriting bind and safe lookup
NCollection_IndexedDataMap.hxx	Added Bound method for conditional bind without overwrite
NCollection_IndexedMap.hxx	Added Added method returning reference to key (new or existing)
AIS_InteractiveContext.cxx	Replaced IsBound+operator() with Seek pattern for status lookups
AIS_ColoredShape.cxx	Replaced IsBound+Bind with TryBound for shape drawer mappings
OpenGl_Context.cxx	Replaced IsBound+Find with Seek for resource lookups
ProjLib_ProjectedCurve.cxx	Optimized map increment pattern using TryBound
BRepTools_WireExplorer.cxx	Replaced IsBound+Bind and IsBound+operator() with TryBound and ChangeSeek
BRepTools_Substitution.cxx	Replaced IsBound+operator() with Seek for shape list lookups
BRepTools_ReShape.cxx	Replaced IsBound+operator() with Seek for replacement lookups
BRepTools_Quilt.cxx	Replaced IsBound+operator() with Seek for shell mappings
BRepTools_PurgeLocations.cxx	Replaced IsBound+Find with Seek pattern in ModifiedShape
BRepTools_Modifier.cxx	Replaced IsBound+Bind with Bind (returns bool indicating new addition)
BRepTools_History.cxx	Replaced IsBound+operator() with Seek for generated/modified lookups
BRepCheck_Wire.cxx	Replaced Contains+Add with Add (returns new index or existing)
BRepCheck_Shell.cxx	Replaced FindIndex+Add pattern with Bound for edge-face mappings
BRepBuilderAPI_Sewing.cxx	Replaced Contains+Add and IsBound+Bind patterns with Add/Bound/TryBound
ShapeUpgrade_UnifySameDomain.cxx	Replaced Contains+Add+ChangeFromKey with Bound pattern
ShapeFix_Shell.cxx	Replaced Contains+Add and IsBound+Find with Add and Seek
ShapeFix.cxx	Replaced Contains+Add pattern with Bound
ShapeAnalysis_Shell.cxx	Optimized duplicate detection using Add return value comparison
Draft_Modification_1.cxx	Replaced Contains+Add patterns with Bound for edge/vertex info maps
BRepOffset_MakeOffset_1.cxx	Replaced IsBound+Bind with Bound for interference pairs
BRepMesh_ModelPreProcessor.cxx	Replaced Contains+Add with Add for face tracking
BRepMesh_ModelPostProcessor.cxx	Replaced Contains+Add pattern with Bound for PCurve mappings
BRepMesh_MeshTool.cxx	Replaced Contains+Add with Add for link tracking
BRepMesh_DiscretFactory.cxx	Replaced IsBound+operator() with Seek for cached function lookups
Geom2dHatch_Hatcher.cxx	Replaced IsBound+ChangeFind with ChangeSeek for hatching updates
LocOpe_Spliter.cxx	Replaced Contains+Add pattern with Bound for face-edge mappings
LocOpe_SplitShape.cxx	Replaced Contains+Add with Add for edge deduplication
LocOpe_Generator.cxx	Replaced Contains+Add and IsBound+Bind with Add and Bound
LocOpe_BuildShape.cxx	Replaced Contains+Add with Add for face deduplication
BRepFeat_Builder.cxx	Replaced IsBound+ChangeFind with ChangeSeek for image lookups
Expr_UnknownIterator.cxx	Replaced Contains+Add with Add for unknown collection
Expr_RUIterator.cxx	Replaced Contains+Add with Add for variable collection
TopOpeBRepDS_FIR.cxx	Replaced Contains+Add pattern with Bound for shape data mapping
TopOpeBRepDS_DataStructure.cxx	Replaced FindIndex+Add with Add for section edges
TopOpeBRepBuild_Tools.cxx	Replaced Contains+Add with Add for processed subshapes
TopOpeBRepBuild_GridSS.cxx	Replaced Contains+Add pattern with Bound for ancestor mappings
TopOpeBRepBuild_Builder1.cxx	Optimized processed parts tracking using Add return value
BRepFill_OffsetWire.cxx	Replaced IsBound+Bind with TryBound and Bound patterns
BRepFill_Generator.cxx	Replaced Contains+Add with Add for modified wire tracking
BRepFill_Evolved.cxx	Replaced nested IsBound+Bind with TryBound for multi-level maps
BRepAlgo_Loop.cxx	Replaced Contains+Add pattern with Bound for vertex-edge mappings
BOPTools_AlgoTools.cxx	Optimized face deduplication using Add return value
BOPAlgo_WireSplitter_1.cxx	Replaced Contains+Add pattern with Bound for edge mappings
BOPAlgo_BOP.cxx	Replaced Contains+Add with Add for shape set deduplication
Storage_Schema.cxx	Replaced IsBound+Find with Seek for type migration lookups
Storage_RootData.cxx	Replaced IsBound+Find with Seek for root lookups
Resource_Manager.cxx	Replaced IsBound+operator() with Seek and Bound patterns
Plugin.cxx	Replaced IsBound+operator() with Seek for cached function lookups
OSD_PerfMeter.cxx	Removed IsBound check before calling print (handled internally)
NCollection_DataMap_Test.cxx	Added TryBound_NoOverwrite test validating preservation semantics
NCollection_IndexedDataMap_Test.cxx	Added 5 Bound* tests validating non-overwriting behavior
NCollection_IndexedMap_Test.cxx	Added 6 Added* tests validating reference return semantics
Poly_MakeLoops.cxx	Replaced IsBound+Find with Seek for node link lookups
MeshTest_CheckTopology.cxx	Replaced Contains+Add pattern with ChangeSeek and Bound
DNaming.cxx	Replaced IsBound+Bind+Find with TryBound for neighbor face mappings
XSControl_TransferReader.cxx	Replaced IsBound+Find with Seek for result lookups
XSAlgo_ShapeProcessor.cxx	Replaced IsBound+Bind with TryBound for parameter maps
MoniTool_Timer.cxx	Replaced IsBound+Find with Seek for timer dictionary
Interface_MSG.cxx	Replaced IsBound+Bind/ChangeFind with TryBound and Bind patterns
IFSelect_SignatureList.cxx	Replaced Contains+ChangeFromKey with Bound for signature counting
XCAFDoc_ShapeTool.cxx	Replaced IsBound+Find and IsBound+Bind with Seek and TryBound
RWMesh_MaterialMap.cxx	Replaced IsBound1+Find1 with Seek1 for material lookups
VrmlData_ShapeConvert.cxx	Replaced IsBound+operator() with Seek for geometry mappings
StepAP214_Protocol.cxx	Replaced IsBound+Find with Seek for type number lookups
STEPCAFControl_Writer.cxx	Replaced IsBound+Find with Seek for label-shape lookups
STEPCAFControl_Reader.cxx	Replaced IsBound+Bind and IsBound+Find with TryBound and Seek
IGESControl_Reader.cxx	Replaced IsBound+Bind/ChangeFind with TryBound and Bound patterns
TPrsStd_DriverTable.cxx	Replaced IsBound+Find with Seek for driver lookups
StdStorage_RootData.cxx	Replaced Contains+FindFromKey with Seek for root lookups
TNaming_Translator.cxx	Replaced IsBound+Find with Seek for copied shape lookups
TNaming_NamedShape.cxx	Replaced IsBound+ChangeFind with ChangeSeek for ref shape updates
TNaming.cxx	Replaced IsBound+operator()/ChangeFind with Seek/ChangeSeek patterns
BinMDF_ADriverTable.cxx	Replaced IsBound+operator() with Seek for driver and ID lookups

[Copilot]

The dereferencing of pId on line 135 occurs inside the null check on line 133, but the logic would be clearer if the Bind call was indented within the if block to make the relationship explicit.

Suggested change

	const int* pId = aStringIdMap.Seek(aTypeName);
	if (pId)
	if (const int* const pId = aStringIdMap.Seek(aTypeName))

[Copilot]

Pull request overview

Copilot reviewed 76 out of 76 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (4)

src/ModelingData/TKBRep/BRepTools/BRepTools_Modifier.cxx:1

• The logic appears inverted. Bind() returns true when the key already exists. The original code used !IsBound() to check if the shape is new. This should likely be if (!myMap.Bind(S, TopoDS_Shape())) to execute the loop only for newly added shapes.
  src/ModelingAlgorithms/TKShHealing/ShapeAnalysis/ShapeAnalysis_Shell.cxx:1
• The duplicate detection logic is incorrect. For NCollection_IndexedMap, Add() returns the index of the key. If the key already exists, it returns the existing index (which can be any value from 1 to Extent). If new, it returns Extent+1 (after insertion). The correct check for a duplicate is dirs.Add(shape) <= aPrevExtent, but this will incorrectly flag a new element added at position 1 as a duplicate when the map was empty. The correct pattern is to compare against aPrevExtent and check if the returned index is <= aPrevExtent (for non-empty maps) or use the return value directly to detect new additions: const int aNewIndex = dirs.Add(shape); if (aNewIndex <= aPrevExtent).
  src/ModelingAlgorithms/TKFeat/LocOpe/LocOpe_BuildShape.cxx:1
• The duplicate detection logic is incorrect. When an IndexedMap is empty (Extent = 0) and a new element is added, it returns index 1. The check mapF.Add(itl.Value()) > aPrevExtent evaluates to 1 > 0 which is true, so it correctly detects new additions. However, this pattern is confusing and fragile. A clearer approach is if (mapF.Add(itl.Value()) == mapF.Extent()) to detect new additions, or store the previous extent and compare the index.
  src/ModelingAlgorithms/TKBO/BOPTools/BOPTools_AlgoTools.cxx:1
• The duplicate detection logic may fail in edge cases. When the map is empty (aPrevExtent = 0), adding a new element returns index 1, so 1 > 0 is true (correct). However, this pattern is not semantically clear. The standard pattern for detecting new additions in IndexedMap is to compare the returned index with Extent() after the addition: const int aNewIdx = aFM.Add(aF); if (aNewIdx == aFM.Extent()) indicates a new addition.

[Copilot]

Pull request overview

Copilot reviewed 76 out of 76 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (3)

src/ModelingData/TKBRep/BRepTools/BRepTools_Modifier.cxx:1

• The refactored code inverts the original logic. Bind() returns true when the key is newly added and false when it already exists (and overwrites). The original code used !myMap.IsBound(S) which executes the block when S is NOT present. The new code with if (myMap.Bind(S, ...)) executes when S IS newly added - this appears correct. However, verify that Bind has the correct semantics (returns true for new binding) as per the NCollection API.
  src/ModelingAlgorithms/TKShHealing/ShapeAnalysis/ShapeAnalysis_Shell.cxx:1
• The comment states 'Add returns index <= previous extent if duplicate' but this is not accurate. For NCollection_IndexedMap, Add() returns the index of the element (new or existing). If the element already exists, it returns its existing index which will be <= previous extent. If it's new, it returns a new index which will be > previous extent. The comment should be corrected to: 'Add returns existing index if duplicate, which will be <= previous extent'.
  src/ModelingData/TKGeomBase/ProjLib/ProjLib_ProjectedCurve.cxx:1
• The expression (*aMap.TryBound(aMapKey, 0))++ is dense and could be less readable. While functionally correct (TryBound returns a pointer to the value, which is dereferenced and incremented), consider extracting this to a temporary variable for clarity: int* pCount = aMap.TryBound(aMapKey, 0); (*pCount)++;