-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Use reflect.Value.Pointer() to compare pointers #1296
base: master
Are you sure you want to change the base?
Conversation
82a2b83
to
356c763
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
356c763
to
2f1c840
Compare
@dolmen quick ping as @AlexanderYastrebov has updated based on your feedback |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docstring for Same
would need to be changed:
Same asserts that two pointers reference the same object. assert.Same(t, ptr1, ptr2) Both arguments must be pointer variables. Pointer variable sameness is determined based on the equality of both type and value.
- This now works for more than pointer variables.
- This now works for types which are convertable to the same type.
- It needs to be clear that only slices pointing to the start of the same array are the same, and not slices pointing to different indices of the same array:
var (
a [8]int
sa0 = a[:]
sa3 = a[3:]
)
assert.Same(t, sa0, sa3)
4aec944
to
e14a11f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a change in behaviour and we must decide if it's safe to do in v1.
No invocation of Same could previously pass with any non-pointer types.
Previously NotSame will always pass if non-pointer type/s are passed it, it can now fail of those reference the same object. In this case I think it would fix a test the author would have intended to fail.
I'd like some more opinions before merging this.
@@ -491,12 +491,12 @@ func validateEqualArgs(expected, actual interface{}) error { | |||
return nil | |||
} | |||
|
|||
// Same asserts that two pointers reference the same object. | |||
// Same asserts that two arguments reference the same object. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is ambiguous for slices in this case:
var (
a [8]int
sa0 = a[:]
sa3 = a[3:]
)
assert.Same(t, sa0, sa3)
sa0 and sa3 reference the same object if you take object to mean array, but not if you mean contiguous segment of an underlying array. The language specification uses both terms at various points. We should make it clear that Same
only considers slices to be the same if they reference the same contiguous segment of an underlying array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO reverse is also surprising
var (
a [8]int
sa0 = a[:]
sa1 = a[:1]
)
assert.Same(t, sa0, sa1) // passes
Summary
Use
reflect.Value.Pointer()
to compare pointersChanges
Updates
samePointers
implementation and adds test casesMotivation
This is needed to check if map, slice and other built-in types point to the same value
Related issues
Fixes #1076