Skip to content
This repository has been archived by the owner on Nov 29, 2018. It is now read-only.

Add js to generic build. #13

Merged
merged 2 commits into from
Jan 14, 2016
Merged

Add js to generic build. #13

merged 2 commits into from
Jan 14, 2016

Conversation

dmitshur
Copy link
Contributor

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
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Contributor Author

I've updated, PTAL.

// Copyright 2011 The Go Authors. All rights reserved.
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file.

// +build !appengine,!gccgo
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you moving these?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of trying to include all remaining tags, use the negation
of the rest of build constraints.

Fixes gopherjs/gopherjs#378.
@klauspost
Copy link
Owner

No problem. Thanks for the contribution!

klauspost added a commit that referenced this pull request Jan 14, 2016
@klauspost klauspost merged commit 999f312 into klauspost:master Jan 14, 2016
@dmitshur dmitshur deleted the patch-1 branch January 14, 2016 17:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants