benbjohnson / immutable

Immutable collections for Go
MIT License
713 stars 32 forks source link

deprecate/remove Builders #36

Closed laher closed 1 year ago

laher commented 1 year ago

I don't think there's any need for the builders if/once this is merged: https://github.com/benbjohnson/immutable/pull/35 . Adding multiple items at once can be achieved without using them.

So, I don't know for sure, but they seem redundant to me now.

Maybe I'm missing something, IDK

BarrensZeppelin commented 1 year ago

Unfortunately the SetMany implementations are incorrect (#39, https://github.com/benbjohnson/immutable/issues/32#issuecomment-1375981222).

I can think of ways to implement SetMany such that it may perform fewer copies in some cases. While the SetMany function is executing, you have to keep track of which nodes are explicitly owned by the receiver map. These nodes are the ones that have been copied/allocated during SetMany, as these cannot be referenced by other maps. For such nodes you can treat the mutable flag as true (but not recursively!). However, this is not simple to implement under the current API, and I think the values that are to be bulk-inserted would have to have some special properties for this to be worthwhile (i.e. they should fall into a small number of hash buckets, otherwise we don't save any allocations at all).

laher commented 1 year ago

After some recent discussions, I still think it's valid to recommend the creation via varargs but I don't know about fully deprecating the builders now. Closing. Ta