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

proposal: time: add method to compare Location values #31686

Closed
rsned opened this issue Apr 25, 2019 · 3 comments
Closed

proposal: time: add method to compare Location values #31686

rsned opened this issue Apr 25, 2019 · 3 comments

Comments

@rsned
Copy link

@rsned rsned commented Apr 25, 2019

What did you do?

I was trying to write some unit tests that my code returned the right *time.Location, but all my attempts to compare the results failed.

https://play.golang.org/p/xQpqTvYQezo

What did you expect to see?

Requesting the same location should have produced two values that would be equal.

x==y seems to just use the pointers, so it's not a deep equal.

Given the private fields in Location, there should be a method on Location that compares the name and the offset to see if they are really equal.

What did you see instead?

Values were not equivalent.

Does this issue reproduce with the latest release (go1.12.4)?

yes

System details

go version go1.12 linux/amd64
GOARCH="amd64"
GOBIN="/usr/lib/google-golang/bin"
GOCACHE="/usr/local/google/home/roberts/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/usr/lib/google-golang/bin/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/lib/google-golang"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/google-golang/pkg/tool/linux_amd64"
GCCGO="/usr/bin/gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
GOROOT/bin/go version: go version go1.12 linux/amd64
GOROOT/bin/go tool compile -V: compile version go1.12
uname -sr: Linux 4.18.10-1rodete2-amd64
Distributor ID:	Debian
Description:	Debian GNU/Linux rodete (upgraded from: Ubuntu 14.04.5 LTS)
Release:	rodete
Codename:	rodete
/lib/x86_64-linux-gnu/libc.so.6: GNU C Library (Debian GLIBC 2.24-12) stable release version 2.24, by Roland McGrath et al.
gdb --version: GNU gdb (GDB) 8.2-gg10
@bradfitz bradfitz changed the title time.Locations are not comparable for equality, should have an Equal(..) method time: add method to compare Location values Apr 25, 2019
@bradfitz bradfitz changed the title time: add method to compare Location values proposal: time: add method to compare Location values Apr 25, 2019
@gopherbot gopherbot added this to the Proposal milestone Apr 25, 2019
@gopherbot gopherbot added the Proposal label Apr 25, 2019
@rsc
Copy link
Contributor

@rsc rsc commented May 1, 2019

It's unclear to me why the String method is not good enough.
The only time it would differ is if someone were intentionally trying to break the comparison.
Doing a deep equal on location would require comparing not just names but also all the many transition moments where the location flips from, say, EST to EDT or back. That's a bit heavy.

@rsned
Copy link
Author

@rsned rsned commented May 1, 2019

My usage for it was I'm writing a library method that returns the Location for a given call, and I wanted to test it, and my naive test on == wasn't working.

Partly, I was surprised that when loading the same location twice, they weren't equal just using ==. I had sort of assumed it would have returned the same thing for the same request, but it looks like it loads from scratch on each call, so the pointers were not the same.

Then I thought, stepping back a bit, what if I used any other library that was written by a malicious or lazy programmer. It would return a Location, but I have no idea if it's returning "true" (e.g. full IANA tzdata transition history, etc) data, or if it was just faking the return with a Fixed Zone that is maybe close enough (LosAngeles being always PST and no PDT), or they were just being mean and return rand.Intn(24)-12. If that was the case, I couldn't just trust String() to be a safe enough check. .

For my case, I trust my code to return the known time.Locations, so using String() suffices for my current case. I was more making the feature request as a search for a longer term robust solution. If you want to close it out or set it as a down the road nice to have thats okay for me.

@rsc
Copy link
Contributor

@rsc rsc commented May 7, 2019

The alternative for this specific case would be to make LoadLocation cache things it has returned and return them again. That would fix == on two different LoadLocation("LosAngeles"). But it would also require keeping those around forever, which gives me pause. Probably no program would ever load too many distinct locations, but especially with LoadLocationFromTZData (which would have to do the same caching), it could in theory be unbounded.

There just doesn't seem to be a nice clean obvious answer here. Combining that with there not being an obvious need, it seems like we should sit this one out and do nothing.

@rsc rsc closed this May 7, 2019
@golang golang locked and limited conversation to collaborators May 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.