klauspost / crc32

CRC32 hash with x64 optimizations
BSD 3-Clause "New" or "Revised" License
74 stars 23 forks source link

Add js to generic build. #13

Closed dmitshur closed 8 years ago

dmitshur commented 8 years ago

Fixes gopherjs/gopherjs#378. /cc @kaustavha

I've tested, and its TestGolden passes using gopherjs compiler after this change:

crc32 $ go test -v
=== RUN   TestGolden
--- PASS: TestGolden (0.00s)
=== RUN   ExampleMakeTable
--- PASS: ExampleMakeTable (0.00s)
PASS
ok      github.com/klauspost/crc32  0.005s

crc32 $ gopherjs test -v
crc32.go:157:10: undeclared name: updateCastagnoli
crc32.go:159:10: undeclared name: updateIEEE
crc32.go:182:48: undeclared name: updateIEEE

crc32 $ nano crc32_generic.go

crc32 $ gopherjs test -v
gopherjs: Source maps disabled. Use Node.js 4.x with source-map-support module for nice stack traces.
=== RUN   TestGolden
--- PASS: TestGolden (0.01s)
PASS
warning: system calls not available, see https://github.com/gopherjs/gopherjs/blob/master/doc/syscalls.md
ok      github.com/klauspost/crc32  0.276s
klauspost commented 8 years ago

This is not your fault (it is like this in the Go sources), but a positive list for the fallback is silly.

Could you check if this works:

// +build !amd64,!amd64p32 appengine gccgo

If it does, let's change this line to this instead.

dmitshur commented 8 years ago

This is not your fault (it is like this in the Go sources), but a positive list for the fallback is silly.

Agreed. I wanted to do that too.

I'll try what you suggested and confirm it works.

dmitshur commented 8 years ago

Ok, I've tested that, and it works for gc (darwin, amd64) and gopherjs (darwin, js) environments:

crc32 $ go test -v
=== RUN   TestGolden
--- PASS: TestGolden (0.00s)
=== RUN   ExampleMakeTable
--- PASS: ExampleMakeTable (0.00s)
PASS
ok      github.com/klauspost/crc32  0.018s
crc32 $ gopherjs test -v
gopherjs: Source maps disabled. Use Node.js 4.x with source-map-support module for nice stack traces.
=== RUN   TestGolden
--- PASS: TestGolden (0.02s)
PASS
ok      github.com/klauspost/crc32  0.587s

The total set of build tags used across all files is quite complex, and I'm not completely confident that every single scenario is covered correctly.

But I'll update the PR. I've also cleaned up the style of build tags to be more idiomatic and consistent.

klauspost commented 8 years ago

The total set of build tags used across all files is quite complex, and I'm not completely confident that every single scenario is covered correctly.

Thanks for testing. At least with this change, we "only" have to exclude the known optimized functions, and white-list all non-optimized platforms.

If you update the PR, I will merge it, and take the blame if it doesn't work :)

dmitshur commented 8 years ago

I've updated, PTAL.

klauspost commented 8 years ago

No problem. Thanks for the contribution!