avoid altering amt when empty #22
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Stebalien
reviewed
Jul 20, 2020
| @@ -3,6 +3,7 @@ package amt | |||
| import ( | |||
| "context" | |||
| "fmt" | |||
| "github.com/stretchr/testify/require" | |||
|
I think I agree with @Stebalien , this value should always be set and never should be serialized as nil. |
amt.go
Outdated
| @@ -495,11 +495,14 @@ func (n *Node) Flush(ctx context.Context, bs cbor.IpldStore, depth int) error { | |||
| if len(n.expVals) == 0 { | |||
| return nil | |||
| } | |||
| n.Bmap = []byte{0} | |||
| n.Bmap = nil | |||
There was a problem hiding this comment.
always defaulting to having the bitfield set would just mean moving this line (before this change) to above the if
|
Fix for byte arrays: whyrusleeping/cbor-gen#29 |
|
Fix merged. |
* Saves an allocation. * Importantly, the bitfield is _always_ initialized. Breaking: This will refuse to decode AMTs with mis-sized bitfields.
|
Fix pushed. Could you take a look at this @whyrusleeping and @acruikshank? |
Kubuxu
approved these changes
Jul 22, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
closes #18
Motivation
Actions like calling
ForEachor adding and removing elements put an empty AMT into a state that causes it to be serialized differently even though it is still the empty array. This is because these actions causeexpValsto be expanded in the node from a nil value to a slice with nil values. WhileexpValsisn't itself serialized, The fact that it is non-nil causes the node'sBmapvalue to be initialized from a nil value to[]byte{0}inFlush, and that does serialize differently.Proposed Changes
Node.Bmaplazily inFlush.This is probably the simplest solution that fixes the problem. It leaves open a bigger question about whether empty nodes should be structurally different when serialized from nodes containing one or more elements. I find it a little surprising, but not really wrong.