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

Bug in fromInt() relating to large unsigned values #92

Closed
pgpnw opened this issue Aug 31, 2020 · 6 comments
Closed

Bug in fromInt() relating to large unsigned values #92

pgpnw opened this issue Aug 31, 2020 · 6 comments

Comments

@pgpnw
Copy link
Contributor

pgpnw commented Aug 31, 2020

Unless I'm mistaken about how fromInt() should work, I think line 128 of long.js, which currently reads:
obj = fromBits(value, (value | 0) < 0 ? -1 : 0, true);

should rather be:
obj = fromBits(value, 0, true);

In its current form, it is sign-extending a value which should be unsigned.

This results in any unsigned integer input in the range 0x80000000 to 0xFFFFFFFF being incorrectly mapped to a Long value in the range 0xFFFFFFFF80000000 to 0xFFFFFFFFFFFFFFFF.

For example:

x = fromInt(0xFFFFFFFE, true); console.log(x);

Produces the output:
Long { low:-2, high:-1, unsigned:true }

In the above output, shouldn't "high" be zero?

@dcodeIO
Copy link
Owner

dcodeIO commented Aug 31, 2020

Yeah, this indeed looks wrong. Would you like to submit a patch?

@pgpnw
Copy link
Contributor Author

pgpnw commented Aug 31, 2020

Sorry. I've just spent roughly the last hour trying to work out how to do that, but I find GitHub to be an utterly confusing mess compared to version control systems I've used in the past.

I created a pull request to make the change, but I think I screwed it up (it seems a pull request isn't what I thought it was), so I closed it and I don't want to mess anything up any further.

@dcodeIO
Copy link
Owner

dcodeIO commented Aug 31, 2020

The easiest way to make a PR is to click "Fork" on the original repository, then check out your fork locally

git clone https://github.com/pgpnw/long.js.git
cd long.js

create a new branch, make your changes and commit the branch to your fork

git checkout -b my-branch
# make changes
git add .
git commit -m "my changes"
git push -u origin my-branch

Now, when visiting the original repo again in the browser, there should be a button to make a PR from your branch on top of master.

Afterwards, if you need to update your fork's master to upstream's master, you can do

git checkout master
git remote add upstream https://github.com/dcodeIO/long.js.git
git fetch upstream
git merge upstream/master

And repeat with a new PR.

I know you didn't ask for this kind of advice, but perhaps it helps as a starting point :)

@pgpnw
Copy link
Contributor Author

pgpnw commented Aug 31, 2020

Do I need to worry about the "Long1" branch? I noticed that the bug is in that branch too, but that version of the source looks like a complete overhaul of the code in the "master" branch.

After the "git checkout -b my-branch" in your instructions, it looks like my branch is based on the "master" branch, rather than the "Long1" branch.

@dcodeIO
Copy link
Owner

dcodeIO commented Aug 31, 2020

You do not have to worry about any other branches than upstream master. The Long1 branch in particular is just the final state of version 1, which isn't relevant anymore.

@srknzl
Copy link

srknzl commented Mar 21, 2021

I wonder why this is idle for a long time. Looks like there is a related pr #94

@dcodeIO dcodeIO closed this as completed Oct 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants