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

Clear() only clears crtl and doesn't clear the groups #20

Closed
rkerno opened this issue Aug 25, 2023 · 1 comment
Closed

Clear() only clears crtl and doesn't clear the groups #20

rkerno opened this issue Aug 25, 2023 · 1 comment

Comments

@rkerno
Copy link

rkerno commented Aug 25, 2023

Clear() is not working as intended. g is a copy of the group, not the group itself. The tests pass because ctrl is cleared, which tricks the test into thinking that clear has worked because the keys can no longer be found. I also noticed that Clear() does not use newEmptyMetadata() so there is an optimisation to cache an empty meta and then just assign it control. The same optimisation could be applied to the groups by creating a zero group and the applying it to each group.

@reltuk reltuk closed this as completed in c0064a0 Aug 28, 2023
@reltuk
Copy link
Contributor

reltuk commented Aug 28, 2023

@rkerno Thanks for the bug report! I've gone ahead and updated the test and pushed a fix for the Clear() implementation. No optimizations there yet --- I agree that keeping an empty metadata around and using that as the right hand argument in the assignments when we need it would be more efficient. Hopefully I can get back to that soon.

kpango pushed a commit to kpango/swiss that referenced this issue Sep 22, 2023
Because groups are using value semantics, the implementation of Clear() was
accidentally clearing a copy of the group instead of the group itself.

Fixes dolthub#20.

Signed-off-by: kpango <kpango@vdaas.org>
userpro pushed a commit to userpro/swiss that referenced this issue Nov 8, 2023
Because groups are using value semantics, the implementation of Clear() was
accidentally clearing a copy of the group instead of the group itself.

Fixes dolthub#20.
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