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

Refactor and new sequence traits, generic sequence operation #3445

Merged
merged 7 commits into from Nov 22, 2021

Conversation

@qingshi163
Copy link
Contributor

@qingshi163 qingshi163 commented Nov 18, 2021

No description provided.

@qingshi163
Copy link
Contributor Author

@qingshi163 qingshi163 commented Nov 19, 2021

Loading

@qingshi163 qingshi163 marked this pull request as ready for review Nov 19, 2021
@qingshi163 qingshi163 changed the title [WIP] Refactor and new sequence traits, generic sequence operation Refactor and new sequence traits, generic sequence operation Nov 19, 2021
@qingshi163 qingshi163 requested a review from youknowone Nov 19, 2021
let mul = sequence::seq_mul(vm, &deque, value)?;
fn _mul(&self, n: isize, vm: &VirtualMachine) -> PyResult<VecDeque<PyObjectRef>> {
let deque = self.borrow_deque();
let n = vm.check_repeat_or_memory_error(deque.len(), n)?;
Copy link
Contributor Author

@qingshi163 qingshi163 Nov 19, 2021

Choose a reason for hiding this comment

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

bit confuse, should it return overflow error rather than memory error?

Loading

Copy link
Contributor Author

@qingshi163 qingshi163 Nov 19, 2021

Choose a reason for hiding this comment

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

after reading the issue 45044 from cpython, I think we should replace all the memory error with overflow error because currently we did not catch the malloc failing.
What do you think? @DimitrisJim @youknowone

Loading

Copy link
Member

@youknowone youknowone Nov 20, 2021

Choose a reason for hiding this comment

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

that sounds like reasonable

Loading

#[inline]
fn _mut_iter_equal_skeleton<F, const SHORT: bool>(
Copy link
Contributor

@Snowapril Snowapril Nov 20, 2021

Choose a reason for hiding this comment

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

This method seems somewhat complex despite it declared as inline. How do you think?

Loading

Copy link
Contributor Author

@qingshi163 qingshi163 Nov 20, 2021

Choose a reason for hiding this comment

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

This one is the skeleton for other related function, only few place is calling it. Also as it is generic, so it should be duplicate for each generic parameters. I mark it inline to try to hint the compiler to optimize out the const bool condition check and the closure it called. I don't know what exactly code the compiler produce but inline is what I expected.

Loading

Copy link
Member

@youknowone youknowone Nov 21, 2021

Choose a reason for hiding this comment

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

I don't think #[inline] make any difference for SHORT. const generic paremeter must be a constant regardless of inlining.

Loading

sequence::cmp(vm, a.boxed_iter(), b.boxed_iter(), op).map(PyComparisonValue::Implemented)
let a = &*zelf.borrow_vec();
let b = &*other.borrow_vec();
// sequence::cmp(vm, a.boxed_iter(), b.boxed_iter(), op).map(PyComparisonValue::Implemented)
Copy link
Member

@youknowone youknowone Nov 21, 2021

Choose a reason for hiding this comment

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

outdated comment?

Loading

if lhs.len() == rhs.len() {
for (a, b) in lhs.zip_eq(rhs) {
if !vm.identical_or_equal(a, b)? {
return Ok(false);
}
}
Ok(true)
} else {
Ok(false)
}
Copy link
Member

@youknowone youknowone Nov 21, 2021

Choose a reason for hiding this comment

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

Suggested change
if lhs.len() == rhs.len() {
for (a, b) in lhs.zip_eq(rhs) {
if !vm.identical_or_equal(a, b)? {
return Ok(false);
}
}
Ok(true)
} else {
Ok(false)
}
if lhs.len() != rhs.len() {
return Ok(false);
}
for (a, b) in lhs.zip_eq(rhs) {
if !vm.identical_or_equal(a, b)? {
return Ok(false);
}
}
Ok(true)

to reduce intent level

Loading

impl SequenceOp<u8> for &str {
fn as_slice(&self) -> &[u8] {
self.as_bytes()
}
}
Copy link
Member

@youknowone youknowone Nov 21, 2021

Choose a reason for hiding this comment

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

I think this doesn't fit the convention of as_slice() - when self is str but return type is &[u8].
The only place this is called is PyStr::mul. I'd rather suggest to use zelf.as_str().as_bytes().mul(..) there than implicitly regarding str as sequence of u8.

Suggested change
impl SequenceOp<u8> for &str {
fn as_slice(&self) -> &[u8] {
self.as_bytes()
}
}

Loading

#[inline]
fn _mut_iter_equal_skeleton<F, const SHORT: bool>(
Copy link
Member

@youknowone youknowone Nov 21, 2021

Choose a reason for hiding this comment

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

I don't think #[inline] make any difference for SHORT. const generic paremeter must be a constant regardless of inlining.

Loading

@qingshi163 qingshi163 merged commit 908b239 into RustPython:main Nov 22, 2021
10 checks passed
Loading
@qingshi163 qingshi163 deleted the main branch Nov 22, 2021
youknowone added a commit to youknowone/RustPython that referenced this issue Nov 26, 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
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants