internal/packing: bug fix: wrong logic for extending a page

Before this fix, 0-sized node could be created when extending the
page.

Updates #2327

internal/packing/packing.go

@@ -305,40 +305,47 @@ func (p *Page) extend(newWidth int, newHeight int) func() {
 	if aborted {
 		origRoot := *p.root
 
-		leftUpper := p.root
+		// Extend the page in the vertical direction.
-		leftLower := &Node{
+		if newHeight-p.height > 0 {
-			x:      0,
+			upper := p.root
-			y:      p.height,
+			lower := &Node{
-			width:  p.width,
+				x:      0,
-			height: newHeight - p.height,
+				y:      p.height,
+				width:  p.width,
+				height: newHeight - p.height,
+			}
+			p.root = &Node{
+				x:      0,
+				y:      0,
+				width:  p.width,
+				height: newHeight,
+				child0: upper,
+				child1: lower,
+			}
+			upper.parent = p.root
+			lower.parent = p.root
 		}
-		left := &Node{
-			x:      0,
-			y:      0,
-			width:  p.width,
-			height: p.height,
-			child0: leftUpper,
-			child1: leftLower,
-		}
-		leftUpper.parent = left
-		leftLower.parent = left
 
-		right := &Node{
+		// Extend the page in the horizontal direction.
-			x:      p.width,
+		if newWidth-p.width > 0 {
-			y:      0,
+			left := p.root
-			width:  newWidth - p.width,
+			right := &Node{
-			height: newHeight,
+				x:      p.width,
+				y:      0,
+				width:  newWidth - p.width,
+				height: newHeight,
+			}
+			p.root = &Node{
+				x:      0,
+				y:      0,
+				width:  newWidth,
+				height: newHeight,
+				child0: left,
+				child1: right,
+			}
+			left.parent = p.root
+			right.parent = p.root
 		}
-		p.root = &Node{
-			x:      0,
-			y:      0,
-			width:  newWidth,
-			height: newHeight,
-			child0: left,
-			child1: right,
-		}
-		left.parent = p.root
-		right.parent = p.root
 
 		origWidth, origHeight := p.width, p.height
 		rollback = func() {

internal/packing/packing_test.go

@@ -327,3 +327,107 @@ func TestNonSquareAlloc(t *testing.T) {
 	p.Free(n0)
 	p.Free(n1)
 }
+
+func TestExtend(t *testing.T) {
+	p := packing.NewPage(1024, 2048)
+	n0 := p.Alloc(1024, 1024)
+	if n0 == nil {
+		t.Errorf("p.Alloc(1024, 1024) failed")
+	}
+
+	// In the current implementation, the page is extended in a horizontal direction first.
+	n1 := p.Alloc(1024, 1024)
+	if n1 == nil {
+		t.Errorf("p.Alloc(1024, 1024) failed")
+	}
+	gotWidth, gotHeight := p.Size()
+	if wantWidth, wantHeight := 2048, 1024; gotWidth != wantWidth || gotHeight != wantHeight {
+		t.Errorf("got: (%d, %d), want: (%d, %d)", gotWidth, gotHeight, wantWidth, wantHeight)
+	}
+
+	// Then, allocating (1024, 2048) fails unfortunately.
+	if p.Alloc(1024, 2048) != nil {
+		t.Errorf("p.Alloc(1024, 2048) must fail but not")
+	}
+
+	n2 := p.Alloc(2048, 1024)
+	if n2 == nil {
+		t.Errorf("p.Alloc(1024, 2048) failed")
+	}
+	gotWidth, gotHeight = p.Size()
+	if wantWidth, wantHeight := 2048, 2048; gotWidth != wantWidth || gotHeight != wantHeight {
+		t.Errorf("got: (%d, %d), want: (%d, %d)", gotWidth, gotHeight, wantWidth, wantHeight)
+	}
+
+	p.Free(n0)
+	p.Free(n1)
+	p.Free(n2)
+}
+
+func TestExtend2(t *testing.T) {
+	p := packing.NewPage(1024, 2048)
+	n0 := p.Alloc(1024, 1024)
+	if n0 == nil {
+		t.Errorf("p.Alloc(1024, 1024) failed")
+	}
+
+	// Extend the page in the both directions. (1024, 2048) should be allocated on the right side.
+	n1 := p.Alloc(1024, 2048)
+	if n1 == nil {
+		t.Errorf("p.Alloc(1024, 2048) failed")
+	}
+	gotWidth, gotHeight := p.Size()
+	if wantWidth, wantHeight := 2048, 2048; gotWidth != wantWidth || gotHeight != wantHeight {
+		t.Errorf("got: (%d, %d), want: (%d, %d)", gotWidth, gotHeight, wantWidth, wantHeight)
+	}
+
+	// There should be a space in the lower-left corner.
+	n2 := p.Alloc(1024, 1024)
+	if n2 == nil {
+		t.Errorf("p.Alloc(1024, 1024) failed")
+	}
+	gotWidth, gotHeight = p.Size()
+	if wantWidth, wantHeight := 2048, 2048; gotWidth != wantWidth || gotHeight != wantHeight {
+		t.Errorf("got: (%d, %d), want: (%d, %d)", gotWidth, gotHeight, wantWidth, wantHeight)
+	}
+
+	p.Free(n0)
+	p.Free(n1)
+	p.Free(n2)
+}
+
+func TestExtend3(t *testing.T) {
+	p := packing.NewPage(1024, 2048)
+
+	// Allocate a small area that doesn't touch the left edge and the bottom edge.
+	// Allocating (1, 1) would split the entire region into left and right in the current implementation,
+	// so allocate (2, 1) here.
+	n0 := p.Alloc(2, 1)
+	if n0 == nil {
+		t.Errorf("p.Alloc(1, 1) failed")
+	}
+
+	// Extend the page in the vertical direction.
+	n1 := p.Alloc(1024, 2047)
+	if n1 == nil {
+		t.Errorf("p.Alloc(1024, 2047) failed")
+	}
+	gotWidth, gotHeight := p.Size()
+	if wantWidth, wantHeight := 1024, 2048; gotWidth != wantWidth || gotHeight != wantHeight {
+		t.Errorf("got: (%d, %d), want: (%d, %d)", gotWidth, gotHeight, wantWidth, wantHeight)
+	}
+
+	// There should be a space on the right side.
+	n2 := p.Alloc(1024, 2048)
+	if n2 == nil {
+		t.Errorf("p.Alloc(1024, 2048) failed")
+	}
+	gotWidth, gotHeight = p.Size()
+	if wantWidth, wantHeight := 2048, 2048; gotWidth != wantWidth || gotHeight != wantHeight {
+		t.Errorf("got: (%d, %d), want: (%d, %d)", gotWidth, gotHeight, wantWidth, wantHeight)
+	}
+
+	p.Free(n0)
+	p.Free(n1)
+	p.Free(n2)
+}