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

Quick fix for 32-bit hosts #244

Merged
merged 1 commit into from Aug 8, 2019

Conversation

@jcburley
Copy link
Contributor

commented Aug 5, 2019

Possible fix for #243.

@rcarmo

This comment has been minimized.

Copy link

commented Aug 5, 2019

Thanks, it appears to have worked:

pi@quad ~/Library/Go/src/github.com/candid82/joker
 % git fetch origin pull/244/head:32bit-fix 
remote: Enumerating objects: 8, done.
remote: Counting objects: 100% (8/8), done.
remote: Compressing objects: 100% (5/5), done.
remote: Total 8 (delta 2), reused 4 (delta 2), pack-reused 0
Unpacking objects: 100% (8/8), done.
From https://github.com/candid82/joker
 * [new ref]         refs/pull/244/head -> 32bit-fix
pi@quad ~/Library/Go/src/github.com/candid82/joker
 % git checkout 32bit-fix
Switched to branch '32bit-fix'
pi@quad ~/Library/Go/src/github.com/candid82/joker
 % ./run.sh --version
v0.12.6

I will test the binary a bit and file other issues should they arise (I've been looking at the source and think I can find my way around at a high level by now).

@candid82

This comment has been minimized.

Copy link
Owner

commented Aug 8, 2019

This will do as a temporary fix until I get to migrating from int to int64 as a base type for Int. Thanks a lot, @jcburley!

@candid82 candid82 merged commit 771141e into candid82:master Aug 8, 2019
1 check passed
1 check passed
ci/circleci Your tests passed on CircleCI!
Details
@jcburley jcburley deleted the jcburley:big-time branch Aug 10, 2019
@jcburley

This comment has been minimized.

Copy link
Contributor Author

commented Aug 18, 2019

This will do as a temporary fix until I get to migrating from int to int64 as a base type for Int.

Ah, here's what this reminds me of:

#174

Perhaps Integer should be introduced as a 32-bit integer on all platforms first (as this seems to be what it is in Clojure+JVM), so people who really want that can migrate to it. (Not sure Joker has the installed base to justify this as a requirement, but it could still be helpful for us to migrate various other things such as tests that are currently commented out and such?)

Then either Int could be redefined as int64, or an explicit Int64 type could be introduced, or possibly both (if someday Int might mean "at least 64 bits", i.e. potentially an int128).

@candid82

This comment has been minimized.

Copy link
Owner

commented Aug 24, 2019

I'd really like to have just one integer type. I do wonder if making Int a wrapper for int64 will affect performance on 32-bit architectures, but then again, performance is an explicit non-goal of Joker (outside of startup time). Still need to look into int -> int64 migration (have been preoccupied with other stuff later).

@jcburley

This comment has been minimized.

Copy link
Contributor Author

commented Aug 24, 2019

I'd really like to have just one integer type. I do wonder if making Int a wrapper for int64 will affect performance on 32-bit architectures, but then again, performance is an explicit non-goal of Joker (outside of startup time). Still need to look into int -> int64 migration (have been preoccupied with other stuff later).

Given that Joker already has a BigInt type, I wonder whether having Int always be 64-bit is really worth the trouble (aside from the possible performance impact, which I suspect would be fairly minor)?

What are the use cases a 64-bit Int enables (on 32-bit systems, since it's already 64-bit on 64-bit systems)?

@candid82

This comment has been minimized.

Copy link
Owner

commented Aug 25, 2019

Having Int the same size on all platforms makes things consistent and predictable and avoids issues like this. That code assume Int to be 64-bit, and I am pretty sure that is not the only place where such assumption is made.

@jcburley

This comment has been minimized.

Copy link
Contributor Author

commented Aug 25, 2019

Having Int the same size on all platforms makes things consistent and predictable and avoids issues like this. That code assume Int to be 64-bit, and I am pretty sure that is not the only place where such assumption is made.

Well, in that case (and other cases we could probably find fairly easily), an easy "fix" could probably be made that returns Int if the result fits, BigInt otherwise.

I'm concerned that changing the semantics of Int at this point would be a lot more work than just fixing problems like that and living with the inconsistency.

From a wider perspective, I'm a bit torn on this issue, because I see advantages to going with the Clojure+JVM "run-anywhere promise", but I don't think Joker (being Go-based) could ever really fulfill that promise to anywhere near the same extent as Clojure+JVM does, since that isn't a design goal of Go.

In other words, yes, theoretically Int might offer Joker users (as well as Joker developers) some more stability as far as that goes; but it's likely they'd experience plenty of other inconsistencies deriving from being hosted in Go (most or all of which reflect Go's favoring of native performance over "compile once, run anywhere").

So the other viewpoint I hold is to think of Joker as a Clojure-like wrapper around the Go language and runtime, in which case having Int == int makes a lot of sense and can be easily explained. Even within the code base, that equivalency should be fairly straightforward to keep in mind.

I prefer the latter viewpoint especially in light of the fact that Clojure+JVM defines no Int type, but rather Integer, which is the JVM-promised (I assume?) "signed 64-bit everywhere" type you're looking for. If Joker had called it Integer in the beginning, there'd be more reason to change it to meet the expectations of Clojure coders.

Regardless of what you decide, I hope to be able to help make it happen.

If Int remains as-is, I can help find and fix places in the code where 32-bit architectures might run into problems, such as the one you identified.

And if you decide to add Integer as int64, I'd like to help with that; or, if you change Int to int64, I can help with that as well.

However, those latter two options (supporting an explicit int64 type) will also require me to do corresponding work on my gostd fork...which I've been looking forward to doing (as it seems strange to have so many int64 values in the Go std converted into BigInt due to the lack of a native Joker type for int64); but I'd have to budget my time between that and upstream work on Joker, of course. (I think I could do the gostd work in just a few days.)

@candid82

This comment has been minimized.

Copy link
Owner

commented Aug 26, 2019

Thank you for such a thoughtful input and for offering help. I've tried converting Int to int64, but that felt a bit like yak shaving. After going back and forth on this, I've decided to keep Int as is. I've also marked problematic places. Most of them are in time and strconv namespaces, and can be easily resolved by using BigInt instead of Int. Others are trickier to resolve without introducing additional range checks at runtime. At this point I am not even sure any of these issues are worth resolving at all. Perhaps it will be sufficient to document that on 32-bit architectures truncation is possible for large integers (that don't fit into 32-bit int). Frankly, I doubt anyone will ever complain about this, and we can always get back to it if it turns out to be a bigger issue than it currently seems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.