Sitelet https://web.archive.org/web/20260604070515/https://github.com/RustPython/RustPython/pull/3580
Skip to content

Fix test_long::test_huge_rshift#3580

Merged
youknowone merged 3 commits into
RustPython:mainfrom
hrchu:fix-test_long_test_huge_rshift
May 3, 2022
Merged

Fix test_long::test_huge_rshift#3580
youknowone merged 3 commits into
RustPython:mainfrom
hrchu:fix-test_long_test_huge_rshift

Conversation

@hrchu
Copy link
Copy Markdown
Contributor

@hrchu hrchu commented Mar 18, 2022 •
edited
Loading

A BigUint is represented as a vector of BigDigits and it seems that in Rust no allocations can have a size larger than isize::MAX. usize::MAX is sufficient for shifting purposes here.

Guided by @youknowone in PyCon APAC 2022 spring sprint

@youknowone
Copy link
Copy Markdown
Member

youknowone commented Mar 20, 2022

I didn't check what's happening in extra_tests/snippets/ints.py, but inner_shift is processing both lshift and rshift. So maybe this test fails for lshift, which have to raise overflow error.

1 similar comment
@youknowone
Copy link
Copy Markdown
Member

youknowone commented Mar 20, 2022

I didn't check what's happening in extra_tests/snippets/ints.py, but inner_shift is processing both lshift and rshift. So maybe this test fails for lshift, which have to raise overflow error.

@hrchu
Copy link
Copy Markdown
Contributor Author

hrchu commented Mar 21, 2022

ok, I'll check it later.
I have some issues with my M1/arm64 environment 🥲

@hrchu
Copy link
Copy Markdown
Contributor Author

hrchu commented Mar 21, 2022

Yes, you are right. I can confirm that the fail case is assert_raises(OverflowError, lambda: 1 << 10 ** 100000)

@youknowone
Copy link
Copy Markdown
Member

youknowone commented Apr 18, 2022

@hrchu did you find the way to distinguish rshift and lshift?

@hrchu
Copy link
Copy Markdown
Contributor Author

hrchu commented Apr 19, 2022

Yes we need to check "number is too large" if the shift op is ""<<"".
The problem is I need two versions of inner_shift with different behavior. Thats stupid.
I still thinking about how to make this more gracefully. Any ideas?

image

@youknowone
Copy link
Copy Markdown
Member

youknowone commented Apr 20, 2022

Will you push the new version to discuss more? I think we don't have much choice. Maybe we can make another small function for shared behavior, but I think having inner_lshift and inner_rshit is hard to avoid without a clever idea.

@hrchu hrchu force-pushed the fix-test_long_test_huge_rshift branch from 45f06d4 to 5c10aed Compare April 24, 2022 13:22
@hrchu
Copy link
Copy Markdown
Contributor Author

hrchu commented Apr 24, 2022

@youknowone pushed. have a look plz 🙏

youknowone
youknowone requested changes Apr 24, 2022
View reviewed changes
Comment thread vm/src/builtins/int.rs Outdated
Comment thread vm/src/builtins/int.rs Outdated
@hrchu hrchu force-pushed the fix-test_long_test_huge_rshift branch from df668d7 to 70e6059 Compare April 28, 2022 11:01
coolreader18
coolreader18 reviewed Apr 28, 2022
View reviewed changes
Comment thread vm/src/builtins/int.rs Outdated
youknowone
youknowone reviewed Apr 28, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@youknowone youknowone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You were talking about 2 duplicated code patterns. I thought you were asking about duplication between them. Now I got it. I think the duplication can be resolved by making them as free functions.

Comment thread vm/src/builtins/int.rs Outdated
Comment thread vm/src/builtins/int.rs Outdated
@hrchu hrchu force-pushed the fix-test_long_test_huge_rshift branch 3 times, most recently from 3ef7915 to e6bf543 Compare May 3, 2022 14:54
@youknowone youknowone force-pushed the fix-test_long_test_huge_rshift branch from e6bf543 to 987d9b9 Compare May 3, 2022 22:49
youknowone
youknowone approved these changes May 3, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@youknowone youknowone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@youknowone youknowone merged commit c303127 into RustPython:main May 3, 2022
@hrchu
Copy link
Copy Markdown
Contributor Author

hrchu commented May 4, 2022

@youknowone thank you for guidance!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coolreader18 coolreader18 coolreader18 left review comments

@youknowone youknowone youknowone approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@hrchu @youknowone @coolreader18