Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

deprecate/remove Builders #36

Closed
laher opened this issue Dec 28, 2022 · 2 comments
Closed

deprecate/remove Builders #36

laher opened this issue Dec 28, 2022 · 2 comments

Comments

@laher
Copy link
Contributor

laher commented Dec 28, 2022

I don't think there's any need for the builders if/once this is merged: #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.

  • We could just delete them from the codebase.
  • Alternatively, just mark them depracted and/or update the Example tests & README guidance

Maybe I'm missing something, IDK

@BarrensZeppelin
Copy link
Contributor

Unfortunately the SetMany implementations are incorrect (#39, #32 (comment)).

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
Copy link
Contributor Author

laher commented Jan 15, 2023

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

@laher laher closed this as completed Jan 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants