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

Default support for the netip package #339

Closed
crawshaw opened this issue Aug 31, 2023 · 1 comment · Fixed by #340
Closed

Default support for the netip package #339

crawshaw opened this issue Aug 31, 2023 · 1 comment · Fixed by #340

Comments

@crawshaw
Copy link
Contributor

Now that netip is in the stdlib, it would be nice if go-cmp supported it by default. Today you need to add:

cmp.Comparer(func(x, y netip.Addr) bool { return x == y }),
cmp.Comparer(func(x, y netip.Prefix) bool { return x == y }),

to make cmp.Diff work with types that include netip types.

@dsnet
Copy link
Collaborator

dsnet commented Aug 31, 2023

At present, cmp has no special treatment of any type in the Go stdlib, including time.Time, which instigated the invention of this package. I think this is a reasonable design goal as special treatment of any particular type begs the question of where to draw the line. Even if we limit cmp to just the stdlib, that's a lot of types that would be linked in.

I see a few options:

  1. Have netip.Addr (and friends) implement an Equal method.
  2. Provide a cmpopts.EquateComparable helper that shallow compares the types passed to it (e.g., cmpopts.EquateComparable(netip.Addr{}, netip.Prefix{})).
  3. Propose a standard way for comparable types to self identify that their comparability is intentional (and guaranteed to not be lost in the future)
  4. Have cmp stop panicking on unexported fields, and just compare fields recursively as it would otherwise.

At minimum, we should probably do option 2 as it makes this easier.

dsnet added a commit that referenced this issue Aug 31, 2023
This helper function makes it easier to specify that comparable types
are safe to directly compare with the == operator in Go.

The API does not use generics as it follows existing options like
cmp.AllowUnexported, cmpopts.IgnoreUnexported, or cmpopts.IgnoreTypes.

While generics provides type safety, the user experience is not as nice.
Our current API allows multiple types to be specified:
	cmpopts.EquateComparable(netip.Addr{}, netip.Prefix{})
While generics would not allow variadic arguments:
	cmpopts.EquateComparable[netip.Addr]()
	cmpopts.EquateComparable[netip.Prefix]()

Fixes #339
dsnet added a commit that referenced this issue Aug 31, 2023
This helper function makes it easier to specify that comparable types
are safe to directly compare with the == operator in Go.

The API does not use generics as it follows existing options like
cmp.AllowUnexported, cmpopts.IgnoreUnexported, or cmpopts.IgnoreTypes.

While generics provides type safety, the user experience is not as nice.
Our current API allows multiple types to be specified:
	cmpopts.EquateComparable(netip.Addr{}, netip.Prefix{})
While generics would not allow variadic arguments:
	cmpopts.EquateComparable[netip.Addr]()
	cmpopts.EquateComparable[netip.Prefix]()

Bump mininimum supported Go to 1.18 for net/netip type.
Start testing on Go 1.21.

Fixes #339
neild pushed a commit that referenced this issue Aug 31, 2023
This helper function makes it easier to specify that comparable types
are safe to directly compare with the == operator in Go.

The API does not use generics as it follows existing options like
cmp.AllowUnexported, cmpopts.IgnoreUnexported, or cmpopts.IgnoreTypes.

While generics provides type safety, the user experience is not as nice.
Our current API allows multiple types to be specified:
	cmpopts.EquateComparable(netip.Addr{}, netip.Prefix{})
While generics would not allow variadic arguments:
	cmpopts.EquateComparable[netip.Addr]()
	cmpopts.EquateComparable[netip.Prefix]()

Bump mininimum supported Go to 1.18 for net/netip type.
Start testing on Go 1.21.

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

Successfully merging a pull request may close this issue.

2 participants