From 2af5d64e2ce4d3b7a6a5b8827b3e12be90c5c4d3 Mon Sep 17 00:00:00 2001
From: Seebs <seebs@seebs.net>
Date: Tue, 19 Mar 2019 20:45:52 -0500
Subject: [PATCH] unbreak a subtle breakage that only test cases could hit

It turns out the logic for "don't update everything if
the incoming slice pointer is the stash" is wrong; it should
really be "don't update everything if the incoming slice
pointer is the one we already have".

The reason this breaks is that one of the tests directly
sets the mapped bit. This breaks my assumption that we'd
never be using the stash and have the mapped bit set, and
that in turn breaks my assumption that the pointer
of an incoming array can't be the stash address unless
we were previously using the stash. If unmap moved us
to non-stashed memory, then a future write could try to
write, notice that it would fit in the stash, copy the
data ... and not update the pointer because the stash
pointer was handled separately.

This way, if you do that, you can end up not using the
stash when you probably could, but you get the expected
behavior. But also, don't set the mapped bit directly.
(I guess there's a good reason to for the test case,
which is using it to verify that unmaps happen when
modifications happen.)

Also the unmap functions should indicate that they have
successfully unmapped, which may help performance in
some test cases.
---
 roaring/container_stash.go | 13 ++++++++-----
 roaring/roaring.go         |  1 +
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/roaring/container_stash.go b/roaring/container_stash.go
index 19164a3b4..9d3f82302 100644
--- a/roaring/container_stash.go
+++ b/roaring/container_stash.go
@@ -111,9 +111,9 @@ func (c *Container) setArray(array []uint16) {
 		return
 	}
 	h := (*reflect.SliceHeader)(unsafe.Pointer(&array))
-	if h.Data == uintptr(unsafe.Pointer(&c.data[0])) {
+	if h.Data == uintptr(unsafe.Pointer(c.pointer)) {
 		// nothing to do but update length
-		c.len, c.cap = int32(h.Len), stashedArraySize
+		c.len = int32(h.Len)
 		return
 	}
 	// array we can fit in data store:
@@ -172,9 +172,9 @@ func (c *Container) setRuns(runs []interval16) {
 		return
 	}
 	h := (*reflect.SliceHeader)(unsafe.Pointer(&runs))
-	if h.Data == uintptr(unsafe.Pointer(&c.data[0])) {
-		// nothing to do but update cap and length
-		c.len, c.cap = int32(h.Len), stashedRunSize
+	if h.Data == uintptr(unsafe.Pointer(c.pointer)) {
+		// nothing to do but update length
+		c.len = int32(h.Len)
 		return
 	}
 
@@ -232,6 +232,7 @@ func (c *Container) unmapArray() {
 	h := (*reflect.SliceHeader)(unsafe.Pointer(&tmp))
 	c.pointer, c.cap = (*uint16)(unsafe.Pointer(h.Data)), int32(h.Cap)
 	runtime.KeepAlive(&tmp)
+	c.mapped = false
 }
 
 // unmapBitmap ensures that the container is not using mmapped storage.
@@ -245,6 +246,7 @@ func (c *Container) unmapBitmap() {
 	h := (*reflect.SliceHeader)(unsafe.Pointer(&tmp))
 	c.pointer, c.cap = (*uint16)(unsafe.Pointer(h.Data)), int32(h.Cap)
 	runtime.KeepAlive(&tmp)
+	c.mapped = false
 }
 
 // unmapRun ensures that the container is not using mmapped storage.
@@ -257,4 +259,5 @@ func (c *Container) unmapRun() {
 	copy(tmp, runs)
 	h := (*reflect.SliceHeader)(unsafe.Pointer(&tmp))
 	c.pointer, c.cap = (*uint16)(unsafe.Pointer(h.Data)), int32(h.Cap)
+	c.mapped = false
 }
diff --git a/roaring/roaring.go b/roaring/roaring.go
index abf45d6dd..836fb307b 100644
--- a/roaring/roaring.go
+++ b/roaring/roaring.go
@@ -1831,6 +1831,7 @@ func (c *Container) runRemove(v uint16) bool {
 		return false
 	}
 	c.unmapRun()
+	runs = c.runs()
 	if v == runs[i].last && v == runs[i].start {
 		runs = append(runs[:i], runs[i+1:]...)
 	} else if v == runs[i].last {