-
Notifications
You must be signed in to change notification settings - Fork 310
Make toBase on Windows behave more like other platforms #482
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
Conversation
|
Thanks for the pull request. Could you modify it to work on 32bit as well? |
|
Sure, do you mean specifically 32-bit windows or other platforms as well? [edit] I cross-compiled for Windows x86 using MSVC, it works still because, even though the intermediate value is 64 bit, its upper bits are all zero so what gets printed is a 'short' hex number |
Other 32bit platforms. I'm also not entirely opposed to making Io 64bit (or greater) only if there is a good reason to do so, but if we go that route, we should make it clear in the docs and throw a clear error when attempting to compile for smaller word size hardware. |
|
I see what you mean. I can try to find an older Linux or BSD and run it under an emulator. As I mentioned above, the Windows version was OK (32-bit app running on 64-bit Windows). I don't think the changes I made will stop it working, as 32-bit systems can do 64-bit arithmetic albeit with some overhead compared to native. I read an article just last week on LWN. It said that the main issue was disappearing hardware; it is basically only embedded systems left now. For io another problem might be finding a version of cmake that would work. Overall I agree there's no point removing 32-bit support just for the sake of it. |
|
I think it's fine to just write it in a way that should work on 32bit without testing it. I'd guess Claude would be able to easily do this. |
|
I tested on an old Raspberry Pi which is ARMv7. It worked (I'm only doing very superficial testing, mind). I am going to slightly change this PR to use uintptr_t instead of uint64_t, so it will be the same size as a pointer on both 32- and 64- bit, which is what is needed. I don't know why Windows used strtoul and everything else uses asDouble (which is implemented by strtod) but I'm assuming there was a good reason for it; best to keep changes to a minimum I think. (sorry this is becoming a bit of a long thread for a 2 line patch!) |
|
Sounds good. Please let me know when it's ready. |
Because on Windows MSVC, unsigned long is 32-bit and on other platforms it is 64-bit, so better to use a type with an explicit size.
|
I've updated and force-pushed it, should be OK now. |
|
Looks good - thanks! |
Description
The message
toBasetreats its receiver, a sequence, as a number formatted as a string, and returns a number formatted as a string in a different base.On windows MSVC it is not able to deal with a number greater than 0xFFFFFFFF because
unsigned longon that platform is 32-bit. This causes a problem foruniqueHexIdbecause the unique id is a pointer-sized (i.e. 64-bit) quantity. Other messages which rely onuniqueHexId, such asasSimpleStringare affected.Type of Change
Related Issues
Fixes #480
Changes Made
The current code parses a string to a number, then converts that number to a string.
strtoullinstead ofstrtoulon Windows only.uint64_t, for intermediate valuesAlso, remove a code comment says "Only base 8 and 16 are currently supported" because this is untrue.
Testing
Test Commands
Before:
After:
Checklist