golang / geo

S2 geometry library in Go
Apache License 2.0
1.69k stars 182 forks source link

Use math/bits to find leading and trailing zeros #35

Closed buth closed 6 years ago

buth commented 6 years ago

Updated the findMSBSetNonZero64 and findLSBSetNonZero64 functions to use the Go standard library and also removed the note about passing zero to findLSBSetNonZero64 having undefined behavior as there is a test that expects the function to return zero in that case.

dsymonds commented 6 years ago

Thanks, though I already considered doing this and decided not to. The big reason is that math/bits is new in Go 1.9 and it'd be nice if this library supported older releases. There isn't much to gain here (unless you have a benchmark showing that this is a big improvement to some operation?) and it's only an internal cleanup. We might do this later though.

buth commented 6 years ago

Maintaining redundancies with the standard library is not a best practice in Go.

What's the reason to actively support an out of date versions of the language? Users tend to vendor dependencies to insulate themselves from breaking changes if they can't upgrade. There's also no such promise made in the project description.

dsymonds commented 6 years ago

There's little to maintain here. This part of the code isn't actively changing. If I was finding it at all burdensome, I would have switched to math/bits already.

It's quite common for people to not to be on the latest version of Go. There's many reasons, such as a slow update cycle inside their company, or using App Engine (which is only just on Go 1.8 now), or using a package manager that lags.

buth commented 6 years ago

Cleaning up code (and making it easier to maintain in the future) seems like a good reason to accept an open source contribution, IMHO.

If you have some unspoken standard on backwards compatibility – or pull requests in general – it would be helpful to add this to your documentation so that would be contributors don't waste their time.