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

Fix addition with negative int64_t #3

Merged
merged 1 commit into from
Jul 14, 2022

Conversation

cerg2010cerg2010
Copy link
Contributor

Negative part wasn't always handled, so expressions like (1 << 256) - 1 behaved incorrectly.

The result of the subtraction contained 5 64-bit numbers, but only first one was correct, i.e. had 0xffff... value. Next 3 parts were all 0's, and the last one was 1.

The fix is simple - just add the negative part of the operand if it was negative initially.

Initial behavior was kinda like unsigned addition, but if (negA) check ruined everything anyways because of incorrect addition of negative part. There's possibly an additional overload needed to have correct unsigned behavior.

Negative part wasn't always handled, so expressions like (1 << 256) - 1
behaved incorrectly.
@tigertv
Copy link
Owner

tigertv commented Jul 13, 2022

Hi, @cerg2010cerg2010!
You are right. There is the trouble. It doesn't work correctly even with (1 << 64) - 1.
I will find out that. Thank you.

@tigertv tigertv self-requested a review July 14, 2022 00:13
@tigertv tigertv added bug Something isn't working good first issue Good for newcomers and removed good first issue Good for newcomers labels Jul 14, 2022
@tigertv tigertv linked an issue Jul 14, 2022 that may be closed by this pull request
@tigertv tigertv removed a link to an issue Jul 14, 2022
@tigertv tigertv merged commit 4707d5f into tigertv:master Jul 14, 2022
@tigertv
Copy link
Owner

tigertv commented Jul 14, 2022

@cerg2010cerg2010, Merged.
Have you seen something terrible else?

@tigertv tigertv removed their request for review July 14, 2022 20:35
@cerg2010cerg2010
Copy link
Contributor Author

@tigertv Thanks.

Yes, there's another problem with bitwise operations - see mekhovyyekulaki@27dae38
After the op amount of parts remains the same, so comparisons start to fail.

Also it would be better if there was an option to disable tests while importing the library, like that - mekhovyyekulaki@4dcf417
They get interferred with the project's tests which make the library less comfortable to use.

All this is implemented in my fork, I can try to split the changes and open the separate pull requests if you'd like.

@tigertv
Copy link
Owner

tigertv commented Jul 15, 2022

@cerg2010cerg2010, OK, make the separate pull requests.

Yes, there's another problem with bitwise operations - see mekhovyyekulaki@27dae38
After the op amount of parts remains the same, so comparisons start to fail.

Would you give a testcase?

Also it would be better if there was an option to disable tests while importing the library, like that - mekhovyyekulaki@4dcf417
They get interferred with the project's tests which make the library less comfortable to use.

No problem. If it's more useful, why not?

This was referenced Jul 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants