Skip to content
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

take address of field for comparison #310

Open
elagergren-spideroak opened this issue Oct 25, 2022 · 4 comments
Open

take address of field for comparison #310

elagergren-spideroak opened this issue Oct 25, 2022 · 4 comments

Comments

@elagergren-spideroak
Copy link

elagergren-spideroak commented Oct 25, 2022

I'm using intsets.Sparse as a field in a struct:

type T struct {
   v intsets.Sparse
   // other fields
}

When traversing T, the field v is copied, causing all methods to fail. This precludes the use of Transformer.

Ideally, there would be an Option that would take the address of the field for use with other Options. Notably, Sparse has an Equals(*Sparse) bool method, so perhaps an alternate idea is to allow different equality methods? But I'm not sure whether go-cmp will take the address of a field in order to invoke the Equal method.

@dsnet

This comment was marked as off-topic.

@elagergren-spideroak

This comment was marked as off-topic.

@dsnet
Copy link
Collaborator

dsnet commented Oct 25, 2022

The fact that values are not always treated as addressable is my biggest regret for the cmp package. I wish it had treated all values as addressable.

I'm sympathetic to adding a fundamental option like:

// ForceAddressable treats all values as if they were addressable.
func ForceAddressable() Option

but would also like to see if it could be enabled by default in a v1.0.0 release of the package.

As a workaround, you can do something like:

// MakeAddressable takes the address of values of T,
// so that methods or options that operate on *T become applicable.
func MakeAddressable[T any]() cmp.Option {
	return cmpopts.AcyclicTransformer("Addr", func(v T) *T {
		return &v
	})
}

and perhaps that's also something we should add to the cmpopts package.

Example usage of MakeAddressable: https://go.dev/play/p/DR371K17bn6

@pohly
Copy link

pohly commented Feb 14, 2023

A similar problem occurs with a struct that contains a atomic.Pointer. I'd like to use a transformer to compare the element that is pointed to instead of the atomic.Pointer value (always different). The transformer itself works:

		cmp.Transformer("podPointer", func(p atomic.Pointer[v1.Pod]) *v1.Pod {
			return p.Load()
		})

But "go vet" (correctly) complains:

func passes lock by value: sync/atomic.Pointer[k8s.io/kubernetes/vendor/k8s.io/api/core/v1.Pod] contains sync/atomic.noCopy

This doesn't work:

		cmp.Transformer("podPointer", func(p *atomic.Pointer[v1.Pod]) *v1.Pod {
			return p.Load()
		})

cmp then doesn't apply the transformation, leading to the original problem:

panic: cannot handle unexported field at {*framework.NodeInfo}.Pods[1].Pod.v:
	"sync/atomic".Pointer[k8s.io/api/core/v1.Pod]
consider using a custom Comparer; if you control the implementation of type, you can also consider using an Exporter, AllowUnexported, or cmpopts.IgnoreUnexported [recovered]

Any suggestions?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants