-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 best_base
to select proper base class
#5324
Conversation
Lib/test/test_types.py
Outdated
@@ -392,8 +392,6 @@ def test(i, format_spec, result): | |||
test(123456, "1=20", '11111111111111123456') | |||
test(123456, "*=20", '**************123456') | |||
|
|||
@unittest.expectedFailure | |||
@run_with_locale('LC_NUMERIC', 'en_US.UTF8') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is not expected to be removed.
vm/src/builtins/int.rs
Outdated
@@ -326,7 +326,8 @@ impl PyInt { | |||
|
|||
#[pyclass( | |||
flags(BASETYPE), | |||
with(PyRef, Comparable, Hashable, Constructor, AsNumber, Representable) | |||
with(PyRef, Comparable, Hashable, Constructor, AsNumber, Representable), | |||
itemsize(BigInt) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get how this is intended to be used yet. How to handle it if it requires multiple fields?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was not intended for multiple types to be declared with itemsize
.
In the pyclass code, if the number of types declared with itemsize is not one, it is planned to raise an error in a way that developers can understand. Unlike basicsize
, it seems difficult to automatically infer item type for each class delcaration, so I have chosen this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm.. I think itemsize
implementation is better to be moved to other PRs. It seems it does not cause wrong behavior in best_base
.
0964777
to
d3e7b31
Compare
I can't reproduce |
Lib/test/test_types.py
Outdated
if sys.platform != "darwin": | ||
test_int__format__locale = unittest.expectedFailure(test_int__format__locale) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverting this will fix the failing test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh my.. That code accidentally got included in the commit. I've fixed it!
Signed-off-by: snowapril <sinjihng@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
debug_assert!(base.is_some()); | ||
Ok(base.unwrap()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unwrap
implies panic!
. So debug_assert!
is duplication.
debug_assert!(base.is_some()); | |
Ok(base.unwrap()) | |
Ok(base.expect("best_base must exist")) |
This revision implements
itemsize
slots and fixbest_base
implementation to select proper base class when generating class.