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.

Sign up for GitHub

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

Jump to bottom

Fix to avoid a compile error due to float to int truncation with GCCGO #9233

Merged
merged 3 commits into from
Dec 19, 2014
Merged

Fix to avoid a compile error due to float to int truncation with GCCGO #9233

merged 3 commits into from
Dec 19, 2014

Conversation

inatatsu
Copy link
Contributor

The unit test pkg/units/size_test.go fails with GCCGO due to a compilation error:

inagaki@black3:~/upstream/src/github.com/docker/docker/pkg/units$ go version
go version gccgo (GCC) 5.0.0 20141029 (experimental) linux/amd64
inagaki@black3:~/upstream/src/github.com/docker/docker/pkg/units$ go test
# _/home/inagaki/upstream/src/github.com/docker/docker/pkg/units
./size_test.go:28:43: error: floating point constant truncated to integer
  assertEquals(t, "2.22 PB", HumanSize(2.22*PB))
                                           ^
FAIL    _/home/inagaki/upstream/src/github.com/docker/docker/pkg/units [build failed]
inagaki@black3:~/upstream/src/github.com/docker/docker/pkg/units$

According to the discussion on GCC Bugzilla, this is due to the difference between GCCGO and GC in the implementations of floating point constant expression. The language specifies that a compiler raises an error when a floating point constant expression is truncated to integer, a precision of the internal representation is implementation-dependent. While my workaround is modifying size_test.go as:

assertEquals(t, "2.22 PB", HumanSize(int64(float64(2.22*PB))))

, is it better to modify the parameter of HumanSize() from int64 to float64 like BytesSize()?
Signed-off-by: Tatsushi Inagaki e29253@jp.ibm.com

@inatatsu
Fix to avoid a compile error due to float to int truncation with GCCGO
967a42f 
Signed-off-by: Tatsushi Inagaki <e29253@jp.ibm.com>
@inatatsu inatatsu mentioned this pull request Nov 19, 2014
Closed
@vbatts vbatts self-assigned this Nov 20, 2014
@crosbymichael
Copy link
Contributor

I would probably change HumanSize to take a float64

@inatatsu
Revert "Fix to avoid a compile error due to float to int truncation w…
36560a7 
…ith GCCGO"

This reverts commit 967a42f.

Signed-off-by: Tatsushi Inagaki <e29253@jp.ibm.com>

Roll back the change to fix the parameter of HumanSize from int64 to float64
@inatatsu
Fix to avoid a compilation error of size_test.go with GCCGO due to fl…
82a5cd0 
…oat to int truncation

Signed-off-by: Tatsushi Inagaki <e29253@jp.ibm.com>
@inatatsu
Copy link
Contributor Author

Modified HumanSize() to take a float64 parameter and added casts to float64 around the arguments for HumanSize() .

@inatatsu
Copy link
Contributor Author

inatatsu commented Dec 1, 2014

@vieux @jfrazelle Do we need additional changes to merge this PR? This problem can occur even with GC depending on the value of floating point constant.

@vbatts
Copy link
Contributor

vbatts commented Dec 11, 2014

LGTM

@inatatsu
Copy link
Contributor Author

@crosbymichael ping?

@crosbymichael
Copy link
Contributor

LGTM

crosbymichael added a commit that referenced this pull request Dec 19, 2014
@crosbymichael
Merge pull request #9233 from inatatsu/fix-pkg-units-size-for-gccgo
2acb856 
Fix to avoid a compile error due to float to int truncation with GCCGO
@crosbymichael crosbymichael merged commit 2acb856 into moby:master Dec 19, 2014
@tiborvass
Copy link
Contributor

@crosbymichael Unit tests are failing, why did we merge this?

@unclejack
Copy link
Contributor

This wasn't needed any more. A much smaller PR was #9715 and it fixed the same problem. This PR introduced a conflict with master once that was merged.

I'm going to send a revert PR for this one.

@unclejack unclejack mentioned this pull request Dec 19, 2014
Merged
@tiborvass
Copy link
Contributor

FYI, instead of reverting this PR, we fixed the compile errors in the unit tests.

@SvenDowideit SvenDowideit mentioned this pull request Jan 27, 2015
Merged
@inatatsu inatatsu deleted the fix-pkg-units-size-for-gccgo branch February 29, 2016 03:38
@thaJeztah thaJeztah mentioned this pull request Mar 30, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@vbatts vbatts

Labels
area/builder
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@inatatsu @crosbymichael @vbatts @tiborvass @unclejack