Skip to content

Commit

Permalink
Revert "Fix compare package for reference types"
Browse files Browse the repository at this point in the history
This reverts commit 033175b.

These changes cause differences to be reported for identical
structures under certain circumstances.
  • Loading branch information
ben-clayton committed Feb 20, 2018
1 parent a72e370 commit 15ce46c
Show file tree
Hide file tree
Showing 3 changed files with 26 additions and 44 deletions.
48 changes: 23 additions & 25 deletions core/data/compare/comparator.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,19 +18,23 @@ import (
"reflect"
"unicode"
"unicode/utf8"
"unsafe"
)

// Comparator is passed to custom comparison functions holding the current comparison context.
type Comparator struct {
Path Path
Handler Handler
seenRef seen // mapping of seen 'reference' pointers to corresponding 'value' pointers
seenVal seen // inverse of the seenRef map
seen seen
custom *Custom
}

type seen map[unsafe.Pointer]unsafe.Pointer
type seen map[seenKey]struct{}

type seenKey struct {
typ reflect.Type
addr1 uintptr
addr2 uintptr
}

func toValue(v reflect.Value, wasPointer bool) interface{} {
if wasPointer {
Expand Down Expand Up @@ -66,28 +70,22 @@ func (t Comparator) AddDiff(reference, value interface{}) {
func (t Comparator) compareValues(v1, v2 reflect.Value, ptr bool) {
// First see if we have seen this comparison already
switch v1.Kind() {
case reflect.Map, reflect.Slice, reflect.Ptr:
if v1.Kind() == v2.Kind() {
addr1 := unsafe.Pointer(v1.Pointer())
addr2 := unsafe.Pointer(v2.Pointer())
if addr1 != unsafe.Pointer(uintptr(0)) && addr2 != unsafe.Pointer(uintptr(0)) {
if seen, ok := t.seenRef[addr1]; ok {
if seen != addr2 {
// Two identical v1 objects are being compared to two distinct v2 objects.
t.Handler(t.Path.Diff(v1.Interface(), v2.Interface()))
}
return // The contents was already compared before.
}
if seen, ok := t.seenVal[addr2]; ok {
if seen != addr1 {
// Two distinct v1 objects are being compared to two identical v2 objects.
t.Handler(t.Path.Diff(v1.Interface(), v2.Interface()))
}
return // The contents was already compared before.
}
t.seenRef[addr1] = addr2
t.seenVal[addr2] = addr1
case reflect.Array, reflect.Map, reflect.Slice, reflect.Struct:
if v1.CanAddr() && v2.CanAddr() {
key := seenKey{
typ: v1.Type(),
addr1: v1.UnsafeAddr(),
addr2: v2.UnsafeAddr(),
}
if key.addr1 > key.addr2 {
// swap for stable ordering
key.addr1, key.addr2 = key.addr2, key.addr1
}
if _, seen := t.seen[key]; seen {
// Already seen, no need to traverse again
return
}
t.seen[key] = struct{}{}
}
}

Expand Down
2 changes: 1 addition & 1 deletion core/data/compare/compare.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ func compare(reference, value interface{}, handler Handler, custom *Custom) {
}
}
}()
t := Comparator{Path: Path{}, Handler: handler, seenRef: seen{}, seenVal: seen{}, custom: custom}
t := Comparator{Path: Path{}, Handler: handler, seen: seen{}, custom: custom}
t.Compare(reference, value)
}

Expand Down
20 changes: 2 additions & 18 deletions core/data/compare/compare_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,11 @@ var (
funcNotNil = func() { funcNil1() } // Not nil.

leafBase = &Object{0, nil}
leafBase2 = &Object{0, nil}
leafEqual = &Object{0, nil}
leafDifferent = &Object{2, nil}

objectBase = &Object{0, []interface{}{leafBase, leafBase2}}
objectEqual1 = &Object{0, []interface{}{leafBase, leafBase2}}
objectBase = &Object{0, []interface{}{leafBase, leafBase}}
objectEqual1 = &Object{0, []interface{}{leafBase, leafBase}}
objectEqual2 = &Object{0, []interface{}{leafEqual, leafBase}}
objectDifferent = &Object{0, []interface{}{leafDifferent, leafBase}}
objectInverse = &Object{0, []interface{}{leafBase, leafEqual}}
Expand Down Expand Up @@ -76,14 +75,6 @@ var (
mapDifferentLength = map[int]string{1: "one"}
mapUint = map[uint]string{1: "one", 2: "two"}

intBase = 42
pointerArrayBase = [2]*int{&intBase, &intBase}
intEqual = 42
pointerArrayEqual = [2]*int{&intEqual, &intEqual}
intDifferent1 = 42
intDifferent2 = 42
pointerArrayDifferent = [2]*int{&intDifferent1, &intDifferent2}

hidden1 = compare.Hidden{Value: Hide{1}}
hidden2 = compare.Hidden{Value: Hide{2}}

Expand Down Expand Up @@ -121,7 +112,6 @@ var (
{"custom 3", Special(1), Special(3), noDiff},
{"special struct custom 1", SpecialStruct{false, 1}, SpecialStruct{false, 1}, noDiff},
{"special struct custom 3", SpecialStruct{false, 1}, SpecialStruct{false, 3}, noDiff},
{"pointer array ==", pointerArrayBase, pointerArrayEqual, noDiff},

// Inequalities
{"int !=", 1, 2, []compare.Path{root.Diff(1, 2)}},
Expand Down Expand Up @@ -221,12 +211,6 @@ var (
root.Member("Value", SpecialStruct{true, 1}, SpecialStruct{true, 3}).
Diff(Special(1), Special(3)),
}},
{"pointer array !=", pointerArrayBase, pointerArrayDifferent, []compare.Path{
root.Index(1, pointerArrayBase, pointerArrayDifferent).Diff(pointerArrayBase[1], pointerArrayDifferent[1]),
}},
{"pointer array != (reversed)", pointerArrayDifferent, pointerArrayBase, []compare.Path{
root.Index(1, pointerArrayDifferent, pointerArrayBase).Diff(pointerArrayDifferent[1], pointerArrayBase[1]),
}},

// Mismatched types
{"int != float64", 1, 1.0, []compare.Path{
Expand Down

0 comments on commit 15ce46c

Please sign in to comment.