Use a Value go type to consolidate

- Changed naming from `extractXThenFree` to `intoX`
- Made all conversions methods on go's `Value`
- Added helpers error(), id(), and bytes() so that
  once we know which it is, we can call the same code
- Added some godocs for each of the `into` methods

ffi/firewood.go

@@ -141,7 +141,7 @@ func (db *Database) Batch(ops []KeyValue) ([]byte, error) {
 		C.size_t(len(ffiOps)),
 		unsafe.SliceData(ffiOps), // implicitly pinned
 	)
-	return extractBytesThenFree(&hash)
+	return (&Value{V: &hash}).intoBytes()
 }
 
 func (db *Database) Propose(keys, vals [][]byte) (*Proposal, error) {
@@ -164,7 +164,7 @@ func (db *Database) Propose(keys, vals [][]byte) (*Proposal, error) {
 		C.size_t(len(ffiOps)),
 		unsafe.SliceData(ffiOps), // implicitly pinned
 	)
-	bytes, id, err := extractBytesAndErrorThenFree(&val)
+	bytes, id, err := (&Value{V: &val}).intoHashAndId()
 	if err != nil {
 		return nil, err
 	}
@@ -187,7 +187,7 @@ func (db *Database) Get(key []byte) ([]byte, error) {
 	values, cleanup := newValueFactory()
 	defer cleanup()
 	val := C.fwd_get_latest(db.handle, values.from(key))
-	bytes, err := extractBytesThenFree(&val)
+	bytes, err := (&Value{V: &val}).intoBytes()
 
 	// If the root hash is not found, return nil.
 	if err != nil && strings.Contains(err.Error(), rootHashNotFound) {
@@ -204,7 +204,7 @@ func (db *Database) Root() ([]byte, error) {
 		return nil, errDBClosed
 	}
 	hash := C.fwd_root_hash(db.handle)
-	bytes, err := extractBytesThenFree(&hash)
+	bytes, err := (&Value{V: &hash}).intoBytes()
 
 	// If the root hash is not found, return a zeroed slice.
 	if err != nil && strings.Contains(err.Error(), rootHashNotFound) {

ffi/memory.go

@@ -27,110 +27,124 @@
 	Value []byte
 }
 
-// extractBytesAndErrorThenFree converts the cgo `Value` payload into either:
-// 1. a 32 byte slice, an id and nil error
-// 2. a nil byte slice, 0 id, and a non-nil error
+// intoHashAndId converts a Value to a 32 byte slice and an id.
 // This should only be called when the `Value` is expected to only contain an error or
 // an ID and a hash, otherwise the behavior is undefined.
-func extractBytesAndErrorThenFree(v *C.struct_Value) ([]byte, uint32, error) {
-	// Pin the returned value to prevent it from being garbage collected.
+//         data    | len   | meaning
+// 1.    | nil     | 0     | invalid
+// 2.    | nil     | non-0 | proposal deleted everything
+// 3.    | non-nil | 0     | error string
+// 4.    | non-nil | non-0 | hash and id
+// TODO: case 2 should be invalid as well, since the proposal should be returning
+// the nil hash, not None.
+
+func (v *Value) intoHashAndId() ([]byte, uint32, error) {
 	defer runtime.KeepAlive(v)
 
-	if v == nil {
+	if v.V == nil {
 		return nil, 0, errNilBuffer
 	}
 
-	// Expected empty case for Rust's `()`
-	// Ignores the length.
-	if v.data == nil {
-		return nil, uint32(v.len), nil
+	if v.V.data != nil {
+		if v.V.len != 0 {
+			// Case 4 - valid bytes returned
+			id := v.id()
+			v.V.len = C.size_t(RootLength)
+			return v.bytes(), id, nil
+		}
+
+		// Case 3 - error string returned
+		return nil, 0, v.error()
 	}
 
-	// If the value is an error string, it should be freed and an error
-	// returned.
-	if v.len == 0 {
-		errStr := C.GoString((*C.char)(unsafe.Pointer(v.data)))
-		C.fwd_free_value(v)
-		return nil, 0, fmt.Errorf("firewood error: %s", errStr)
+	// Case 2 - proposal deleted everything
+	if v.id() > 0 {
+		return nil, v.id(), nil
 	}
 
-	// We must assume that the byte slice is a valid root slice.
-	id := uint32(v.len)
-	buf := C.GoBytes(unsafe.Pointer(v.data), RootLength)
-	v.len = C.size_t(RootLength) // set the length to clean
-	C.fwd_free_value(v)
-	return buf, id, nil
+	// Case 1 - invalid value
+	return nil, 0, errBadValue
+}
+
+// Value is a wrapper around the cgo `Value` struct.
+// It is used so that the conversions are methods on the Value struct.
+type Value struct {
+	V *C.struct_Value
 }
 
-// extractErrorThenFree converts the cgo `Value` payload to either:
-// 1. a nil value, indicating no error, or
-// 2. a non-nil error, indicating an error occurred.
-// This should only be called when the `Value` is expected to only contain an error.
-// Otherwise, an error is returned.
-func extractErrorThenFree(v *C.struct_Value) error {
-	// Pin the returned value to prevent it from being garbage collected.
+func (v *Value) intoError() error {
 	defer runtime.KeepAlive(v)
 
-	if v == nil {
+	if v.V == nil {
 		return errNilBuffer
 	}
 
-	// Expected empty case for Rust's `()`
-	// Ignores the length.
-	if v.data == nil {
+	if v.V.data == nil {
 		return nil
 	}
 
-	// If the value is an error string, it should be freed and an error
-	// returned.
-	if v.len == 0 {
-		errStr := C.GoString((*C.char)(unsafe.Pointer(v.data)))
-		C.fwd_free_value(v)
-		return fmt.Errorf("firewood error: %s", errStr)
-	}
+	return v.error()
+}
+
+// error returns the error string from the cgo `Value` payload.
+// This avoids code duplication
+// input must be validated -- v not nil and v.V.data must be non-nil.
+func (v *Value) error() error {
+	errStr := C.GoString((*C.char)(unsafe.Pointer(v.V.data)))
+	C.fwd_free_value(v.V)
+	return fmt.Errorf("firewood error: %s", errStr)
+}
 
-	// The value is formatted incorrectly.
-	// We should still attempt to free the value.
-	C.fwd_free_value(v)
-	return errBadValue
+// bytes returns the byte slice from the cgo `Value` payload.
+// This avoids code duplication
+// input must be validated -- v not nil and v.V.data must be non-nil, and v.V.len must be non-zero.
+func (v *Value) bytes() []byte {
+	// This assertion should have been done in the caller
+	// if v.V.data == nil || v.V.len == 0 {
+	// 	panic("firewood error: invalid value")
+	// }
+
+	buf := C.GoBytes(unsafe.Pointer(v.V.data), C.int(v.V.len))
+	C.fwd_free_value(v.V)
+	return buf
 }
 
-// extractBytesThenFree converts the cgo `Value` payload to either:
-// 1. a non-nil byte slice and nil error, indicating a valid byte slice
-// 2. a nil byte slice and nil error, indicating an empty byte slice
-// 3. a nil byte slice and a non-nil error, indicating an error occurred.
-// This should only be called when the `Value` is expected to only contain an error or a byte slice.
-// Otherwise, an error is returned.
-func extractBytesThenFree(v *C.struct_Value) ([]byte, error) {
-	// Pin the returned value to prevent it from being garbage collected.
+func (v *Value) id() uint32 {
+	return uint32(v.V.len)
+}
+
+// intoBytes converts a Value to a byte or an error
+// based on the following table:
+//
+//	| data    | len   | meaning
+//
+// 1.    | nil     | 0     | empty
+// 2.    | nil     | non-0 | invalid
+// 3.    | non-nil | 0     | error string
+// 4.    | non-nil | non-0 | bytes (most common)
+func (v *Value) intoBytes() ([]byte, error) {
 	defer runtime.KeepAlive(v)
 
-	if v == nil {
+	if v.V == nil {
 		return nil, errNilBuffer
 	}
 
-	// Expected behavior - no data and length is zero.
-	if v.len == 0 && v.data == nil {
-		return nil, nil
+	// Case 4 - valid bytes returned
+	if v.V.len != 0 && v.V.data != nil {
+		return v.bytes(), nil
 	}
 
-	// Normal case, data is non-nil and length is non-zero.
-	if v.len != 0 && v.data != nil {
-		buf := C.GoBytes(unsafe.Pointer(v.data), C.int(v.len))
-		C.fwd_free_value(v)
-		return buf, nil
+	// Case 1 - empty value
+	if v.V.len == 0 && v.V.data == nil {
+		return nil, nil
 	}
 
-	// Data non-nil but length is zero indcates an error.
-	if v.len == 0 {
-		errStr := C.GoString((*C.char)(unsafe.Pointer(v.data)))
-		C.fwd_free_value(v)
-		return nil, fmt.Errorf("firewood error: %s", errStr)
+	// Case 3 - error string returned
+	if v.V.data != nil {
+		return nil, v.error()
 	}
 
-	// The value is formatted incorrectly.
-	// We should still attempt to free the value.
-	C.fwd_free_value(v)
+	// Case 2 - invalid value
 	return nil, errBadValue
 }
 

ffi/proposal.go

@@ -66,7 +66,7 @@ func (p *Proposal) Get(key []byte) ([]byte, error) {
 
 	// Get the value for the given key.
 	val := C.fwd_get_from_proposal(p.handle, C.uint32_t(p.id), values.from(key))
-	return extractBytesThenFree(&val)
+	return (&Value{V: &val}).intoBytes()
 }
 
 // Propose creates a new proposal with the given keys and values.
@@ -101,7 +101,7 @@ func (p *Proposal) Propose(keys, vals [][]byte) (*Proposal, error) {
 		C.size_t(len(ffiOps)),
 		unsafe.SliceData(ffiOps),
 	)
-	bytes, id, err := extractBytesAndErrorThenFree(&val)
+	bytes, id, err := (&Value{V: &val}).intoHashAndId()
 	if err != nil {
 		return nil, err
 	}
@@ -126,7 +126,7 @@ func (p *Proposal) Commit() error {
 
 	// Commit the proposal and return the hash.
 	errVal := C.fwd_commit(p.handle, C.uint32_t(p.id))
-	err := extractErrorThenFree(&errVal)
+	err := (&Value{V: &errVal}).intoError()
 	if err != nil {
 		// this is unrecoverable due to Rust's ownership model
 		// The underlying proposal is no longer valid.
@@ -150,5 +150,5 @@ func (p *Proposal) Drop() error {
 	// Drop the proposal.
 	val := C.fwd_drop_proposal(p.handle, C.uint32_t(p.id))
 	p.id = 0
-	return extractErrorThenFree(&val)
+	return (&Value{V: &val}).intoError()
 }

ffi/revision.go

@@ -44,7 +44,7 @@ func newRevision(handle *C.DatabaseHandle, root []byte) (*Revision, error) {
 	values, cleanup := newValueFactory()
 	defer cleanup()
 	val := C.fwd_get_from_root(handle, values.from(root), values.from([]byte{}))
-	_, err := extractBytesThenFree(&val)
+	_, err := (&Value{V: &val}).intoBytes()
 	if err != nil {
 		// Any error from this function indicates that the root is inaccessible.
 		return nil, errRevisionNotFound
@@ -69,7 +69,7 @@ func (r *Revision) Get(key []byte) ([]byte, error) {
 	defer cleanup()
 
 	val := C.fwd_get_from_root(r.handle, values.from(r.root), values.from(key))
-	value, err := extractBytesThenFree(&val)
+	value, err := (&Value{V: &val}).intoBytes()
 	if err != nil {
 		// Any error from this function indicates that the revision is inaccessible.
 		r.root = nil