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

Various lints #80

Merged
merged 18 commits into from
Jul 3, 2018
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion Gopkg.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 5 additions & 5 deletions basic_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import (
)

func TestBasic(t *testing.T) {
var tree *Tree = NewTree(nil, 0)
tree := NewTree(nil, 0)
up := tree.Set([]byte("1"), []byte("one"))
if up {
t.Error("Did not expect an update (should have been create)")
Expand Down Expand Up @@ -220,7 +220,7 @@ func TestIntegration(t *testing.T) {
}

records := make([]*record, 400)
var tree *Tree = NewTree(nil, 0)
tree := NewTree(nil, 0)

randomRecord := func() *record {
return &record{randstr(20), randstr(20)}
Expand Down Expand Up @@ -302,7 +302,7 @@ func TestIterateRange(t *testing.T) {
}
sort.Strings(keys)

var tree *Tree = NewTree(nil, 0)
tree := NewTree(nil, 0)

// insert all the data
for _, r := range records {
Expand Down Expand Up @@ -394,7 +394,7 @@ func TestProof(t *testing.T) {

// Construct some random tree
db := db.NewMemDB()
var tree *VersionedTree = NewVersionedTree(db, 100)
tree := NewVersionedTree(db, 100)
for i := 0; i < 1000; i++ {
key, value := randstr(20), randstr(20)
tree.Set([]byte(key), []byte(value))
Expand Down Expand Up @@ -423,7 +423,7 @@ func TestProof(t *testing.T) {

func TestTreeProof(t *testing.T) {
db := db.NewMemDB()
var tree *Tree = NewTree(db, 100)
tree := NewTree(db, 100)

// should get false for proof with nil root
_, _, err := tree.GetWithProof([]byte("foo"))
Expand Down
8 changes: 6 additions & 2 deletions doc.go
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
// Package iavl implements a versioned, snapshottable (immutable) AVL+ tree
// for persisting key-value pairs.
//
//
// Basic usage of VersionedTree.
//
// import "github.com/tendermint/iavl"
Expand All @@ -23,12 +27,12 @@
// Proof of existence:
//
// root := tree.Hash()
// val, proof, err := tree.GetVersionedWithProof([]byte("bob"), 2) // "xyz", KeyProof, nil
// val, proof, err := tree.GetVersionedWithProof([]byte("bob"), 2) // "xyz", RangeProof, nil
// proof.Verify([]byte("bob"), val, root) // nil
//
// Proof of absence:
//
// _, proof, err = tree.GetVersionedWithProof([]byte("tom"), 2) // nil, KeyProof, nil
// _, proof, err = tree.GetVersionedWithProof([]byte("tom"), 2) // nil, RangeProof, nil
// proof.Verify([]byte("tom"), nil, root) // nil
//
// Now we delete an old version:
Expand Down
162 changes: 77 additions & 85 deletions node.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,46 +41,50 @@ func NewNode(key []byte, value []byte, version int64) *Node {

// MakeNode constructs an *Node from an encoded byte slice.
//
// The new node doesn't have its hash saved or set. The caller must set it
// The new node doesn't have its hash saved or set. The caller must set it
// afterwards.
func MakeNode(buf []byte) (node *Node, err cmn.Error) {
node = &Node{}
n := 0
cause := error(nil)
func MakeNode(buf []byte) (*Node, cmn.Error) {

// Read node header.

node.height, n, cause = amino.DecodeInt8(buf)
// Read node header (height, size, version, key).
height, n, cause := amino.DecodeInt8(buf)
if cause != nil {
return nil, cmn.ErrorWrap(cause, "decoding node.height")
}
buf = buf[n:]

node.size, n, cause = amino.DecodeVarint(buf)
size, n, cause := amino.DecodeVarint(buf)
if cause != nil {
return nil, cmn.ErrorWrap(cause, "decoding node.size")
}
buf = buf[n:]

node.version, n, cause = amino.DecodeVarint(buf)
ver, n, cause := amino.DecodeVarint(buf)
if cause != nil {
return nil, cmn.ErrorWrap(cause, "decoding node.version")
}
buf = buf[n:]

node.key, n, cause = amino.DecodeByteSlice(buf)
key, n, cause := amino.DecodeByteSlice(buf)
if cause != nil {
return nil, cmn.ErrorWrap(cause, "decoding node.key")
}
buf = buf[n:]

node := &Node{
height: height,
size: size,
version: ver,
key: key,
}

// Read node body.

if node.isLeaf() {
node.value, _, cause = amino.DecodeByteSlice(buf)
val, _, cause := amino.DecodeByteSlice(buf)
if cause != nil {
return nil, cmn.ErrorWrap(cause, "decoding node.value")
}
node.value = val
} else { // Read children.
leftHash, n, cause := amino.DecodeByteSlice(buf)
if cause != nil {
Expand Down Expand Up @@ -145,9 +149,8 @@ func (node *Node) has(t *Tree, key []byte) (has bool) {
}
if bytes.Compare(key, node.key) < 0 {
return node.getLeftNode(t).has(t, key)
} else {
return node.getRightNode(t).has(t, key)
}
return node.getRightNode(t).has(t, key)
}

// Get a key under the node.
Expand All @@ -165,31 +168,28 @@ func (node *Node) get(t *Tree, key []byte) (index int64, value []byte) {

if bytes.Compare(key, node.key) < 0 {
return node.getLeftNode(t).get(t, key)
} else {
rightNode := node.getRightNode(t)
index, value = rightNode.get(t, key)
index += node.size - rightNode.size
return index, value
}
rightNode := node.getRightNode(t)
index, value = rightNode.get(t, key)
index += node.size - rightNode.size
return index, value
}

func (node *Node) getByIndex(t *Tree, index int64) (key []byte, value []byte) {
if node.isLeaf() {
if index == 0 {
return node.key, node.value
} else {
return nil, nil
}
} else {
// TODO: could improve this by storing the
// sizes as well as left/right hash.
leftNode := node.getLeftNode(t)
if index < leftNode.size {
return leftNode.getByIndex(t, index)
} else {
return node.getRightNode(t).getByIndex(t, index-leftNode.size)
}
return nil, nil
}
// TODO: could improve this by storing the
// sizes as well as left/right hash.
leftNode := node.getLeftNode(t)

if index < leftNode.size {
return leftNode.getByIndex(t, index)
}
return node.getRightNode(t).getByIndex(t, index-leftNode.size)
}

// Computes the hash of the node without computing its descendants. Must be
Expand Down Expand Up @@ -387,35 +387,33 @@ func (node *Node) set(t *Tree, key []byte, value []byte) (

if updated {
return node, updated, orphaned
} else {
node.calcHeightAndSize(t)
newNode, balanceOrphaned := node.balance(t)
return newNode, updated, append(orphaned, balanceOrphaned...)
}
node.calcHeightAndSize(t)
newNode, balanceOrphaned := node.balance(t)
return newNode, updated, append(orphaned, balanceOrphaned...)
}
}

// newHash/newNode: The new hash or node to replace node after remove.
// newKey: new leftmost leaf key for tree after successfully removing 'key' if changed.
// value: removed value.
func (node *Node) remove(t *Tree, key []byte) (
newHash []byte, newNode *Node, newKey []byte, value []byte, orphaned []*Node,
) {
// removes the node corresponding to the passed key and balances the tree.
// It returns:
// - the hash of the new node (or nil if the node is the one removed)
// - the node that replaces the orig. node after remove
// - new leftmost leaf key for tree after successfully removing 'key' if changed.
// - the removed value
// - the orphaned nodes.
func (node *Node) remove(t *Tree, key []byte) ([]byte, *Node, []byte, []byte, []*Node) {
version := t.version + 1

if node.isLeaf() {
if bytes.Equal(key, node.key) {
return nil, nil, nil, node.value, []*Node{node}
}
return node.hash, node, nil, nil, orphaned
return node.hash, node, nil, nil, nil
}

// node.key < key; we go to the left to find the key:
if bytes.Compare(key, node.key) < 0 {
var newLeftHash []byte
var newLeftNode *Node

newLeftHash, newLeftNode, newKey, value, orphaned =
node.getLeftNode(t).remove(t, key)
newLeftHash, newLeftNode, newKey, value, orphaned := node.getLeftNode(t).remove(t, key)

if len(orphaned) == 0 {
return node.hash, node, nil, value, orphaned
Expand All @@ -430,30 +428,26 @@ func (node *Node) remove(t *Tree, key []byte) (
newNode, balanceOrphaned := newNode.balance(t)

return newNode.hash, newNode, newKey, value, append(orphaned, balanceOrphaned...)
} else {
var newRightHash []byte
var newRightNode *Node

newRightHash, newRightNode, newKey, value, orphaned =
node.getRightNode(t).remove(t, key)

if len(orphaned) == 0 {
return node.hash, node, nil, value, orphaned
} else if newRightHash == nil && newRightNode == nil { // right node held value, was removed
return node.leftHash, node.leftNode, nil, value, orphaned
}
orphaned = append(orphaned, node)
}
// node.key >= key; either found or look to the right:
newRightHash, newRightNode, newKey, value, orphaned := node.getRightNode(t).remove(t, key)

newNode := node.clone(version)
newNode.rightHash, newNode.rightNode = newRightHash, newRightNode
if newKey != nil {
newNode.key = newKey
}
newNode.calcHeightAndSize(t)
newNode, balanceOrphaned := newNode.balance(t)
if len(orphaned) == 0 {
return node.hash, node, nil, value, orphaned
} else if newRightHash == nil && newRightNode == nil { // right node held value, was removed
return node.leftHash, node.leftNode, nil, value, orphaned
}
orphaned = append(orphaned, node)

return newNode.hash, newNode, nil, value, append(orphaned, balanceOrphaned...)
newNode := node.clone(version)
newNode.rightHash, newNode.rightNode = newRightHash, newRightNode
if newKey != nil {
newNode.key = newKey
}
newNode.calcHeightAndSize(t)
newNode, balanceOrphaned := newNode.balance(t)

return newNode.hash, newNode, nil, value, append(orphaned, balanceOrphaned...)
}

func (node *Node) getLeftNode(t *Tree) *Node {
Expand Down Expand Up @@ -531,34 +525,32 @@ func (node *Node) balance(t *Tree) (newSelf *Node, orphaned []*Node) {
// Left Left Case
newNode, orphaned := node.rotateRight(t)
return newNode, []*Node{orphaned}
} else {
// Left Right Case
var leftOrphaned *Node
}
// Left Right Case
var leftOrphaned *Node

left := node.getLeftNode(t)
node.leftHash = nil
node.leftNode, leftOrphaned = left.rotateLeft(t)
newNode, rightOrphaned := node.rotateRight(t)
left := node.getLeftNode(t)
node.leftHash = nil
node.leftNode, leftOrphaned = left.rotateLeft(t)
newNode, rightOrphaned := node.rotateRight(t)

return newNode, []*Node{left, leftOrphaned, rightOrphaned}
}
return newNode, []*Node{left, leftOrphaned, rightOrphaned}
}
if balance < -1 {
if node.getRightNode(t).calcBalance(t) <= 0 {
// Right Right Case
newNode, orphaned := node.rotateLeft(t)
return newNode, []*Node{orphaned}
} else {
// Right Left Case
var rightOrphaned *Node
}
// Right Left Case
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd advocate that sometimes if then else does make perfect sense and this is one of those cases

if left {
} else {
  (right)
}

NOT

if left {
}

(right)

Copy link
Contributor Author

@liamsi liamsi Jul 3, 2018

Choose a reason for hiding this comment

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

I kinda agree. It makes more sense here then in many other places. I think the point is that there is a return in the first case (which makes the else obsolete semantically). Can reintroduce if you think that is important.
I would rather suggest to refactor this such that the return statement(s) is(/are) outside of the if-then-else. Something I really like about (idiomatic) go is that code usually looks very similar. For me an if left { // ...} else { //right } would look like there are no return statements to expect there. What do you think?

var rightOrphaned *Node

right := node.getRightNode(t)
node.rightHash = nil
node.rightNode, rightOrphaned = right.rotateRight(t)
newNode, leftOrphaned := node.rotateLeft(t)
right := node.getRightNode(t)
node.rightHash = nil
node.rightNode, rightOrphaned = right.rotateRight(t)
newNode, leftOrphaned := node.rotateLeft(t)

return newNode, []*Node{right, leftOrphaned, rightOrphaned}
}
return newNode, []*Node{right, leftOrphaned, rightOrphaned}
}
// Nothing changed
return node, []*Node{}
Expand Down
Loading