ice_nine icon indicating copy to clipboard operation
ice_nine copied to clipboard

Replace Hash with Set in RecursionGuard.

Open troessner opened this issue 8 years ago • 3 comments

While browsing IceNine's codebase I noticed that we can replace the Hash in RecursionGuard with a better fitting Set since we don't need the values.

troessner avatar May 01 '16 20:05 troessner

@troessner I think there was a reason to use the object ids, but we'll have to wait for @dkubb to find the time to answer this.

mbj avatar May 01 '16 20:05 mbj

I do not recall the details on why we track the object ids instead of the objects themselves. But I've got some thoughts on the approach.

Lets focus on the semantical difference: Tracking the object id, allows the object to be GCed. Turning the object id still in the recursion guard into an orphan.

This could lead to the memory allocator to allocate a new object that would resolve to the same id. While unlikely this seems still to be a lingering correctness issue that should be addressed, via tracking objects not their ids.

mbj avatar May 02 '16 14:05 mbj

OT: I'd even go as far and assume that when somebody wants to freeze a cyclic, or "need to be GCed while frozen" datastructure a generic deep freeze like ice_nine is the wrong thing to do.

I'd even support raising an exception on cycles.

mbj avatar May 02 '16 14:05 mbj