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

SetIfAbsent #77

Merged
merged 5 commits into from
Dec 14, 2020
Merged

SetIfAbsent #77

merged 5 commits into from
Dec 14, 2020

Conversation

ZenGround0
Copy link
Contributor

Closes #75 with the SetIfAbsent approach

  • modifyValue now returns a boolean indicating whether or not it has changed the subtree
  • modifyValue now takes in an overwrite boolean which only allows overwriting existing keys when true
  • SetIfAbsent exported function

Tests still needed

Copy link
Member

@anorth anorth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, needs tests.

The boolean constants now flying around can be a little hard to follow. Consider adding two new boolean type wrappers for N/OVERWRITE for the parameter and UN/MODIFIED for the return value, so their meaning is transparent wherever they are used.

@rvagg
Copy link
Member

rvagg commented Dec 2, 2020

I had a bit of a go of extending this but pushing the MarshalCBOR right down to where the value is set - so it's not wasted if the value isn't set. But nil gets in the way of doing that, it'd probably require an additional argument to modifyValue() to make it work nicely. That's an option to reduce some code duplication and avoid unnecessary encode cost if the "is not set" case is going to be common enough.

@ZenGround0 ZenGround0 marked this pull request as ready for review December 7, 2020 17:33
@ZenGround0 ZenGround0 requested a review from anorth December 7, 2020 17:34
Copy link
Member

@anorth anorth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after nits

hamt.go Outdated
@@ -19,6 +19,26 @@ import (
const bucketSize = 3
const defaultBitWidth = 8

//-----------------------------------------------------------------------------
// Boolean constants
type overwrite = bool
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the compiler to help us use these correctly, could you remove the = here and define the consts like OVERWRITE = overwrite(false)?

hamt.go Outdated

// SetIfAbsent sets key k to value v only if k is not already set to some value.
// Returns true if the value mapped to k is changed by this operation
// is set, false otherwise.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// is set, false otherwise.
// false otherwise.

hamt.go Outdated
@@ -575,17 +625,17 @@ func (n *Node) cleanChild(chnd *Node, cindex byte) error {
// cleanNode()). Recursive calls use the same arguments on child nodes but
// note that `hv.Next()` is not idempotent. Each call will increment the number
// of bits chomped off the hash digest for this key.
func (n *Node) modifyValue(ctx context.Context, hv *hashBits, k []byte, v *cbg.Deferred) error {
func (n *Node) modifyValue(ctx context.Context, hv *hashBits, k []byte, v *cbg.Deferred, overwrite bool) (modified, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
func (n *Node) modifyValue(ctx context.Context, hv *hashBits, k []byte, v *cbg.Deferred, overwrite bool) (modified, error) {
func (n *Node) modifyValue(ctx context.Context, hv *hashBits, k []byte, v *cbg.Deferred, replace overwrite) (modified, error) {

hamt.go Outdated
}
}
return ErrNotFound
return false, ErrNotFound
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return false, ErrNotFound
return UNMODIFIED, ErrNotFound

hamt_test.go Outdated
val1 := []byte("owl bear")
key := "favorite-animal"
success, err := begn.SetIfAbsent(ctx, key, val1)
if err != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We depend on testify already, so require.NoError(err).

@ZenGround0 ZenGround0 merged commit d1f554a into master Dec 14, 2020
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

Successfully merging this pull request may close these issues.

Set() to return a value indicating whether the key already existed
5 participants