Fix some set operation to compare self with args #3912
Conversation
kth496
force-pushed
the
improve-inplace-op
branch
from
d5e969b to
ada0063
Compare
|
It is up to you, but you don't need to put everything in single PR. You can submit splat small PRs for each functions. |
vm/src/builtins/set.rs
Outdated
| #[pymethod(magic)] | ||
| fn iand(zelf: PyRef<Self>, set: AnySet, vm: &VirtualMachine) -> PyResult<PyRef<Self>> { | ||
| fn iand(zelf: PyRef<Self>, set: PyObjectRef, vm: &VirtualMachine) -> PyResult<PyRef<Self>> { | ||
| if zelf.get_id() == set.get_id() { |
There was a problem hiding this comment.
| if zelf.get_id() == set.get_id() { | |
| if zelf.is(&set) { |
vm/src/builtins/set.rs
Outdated
| if zelf.get_id() == set.get_id() { | ||
| return Ok(zelf); | ||
| } |
There was a problem hiding this comment.
intersection_update also needs to have a check like this. As iand and intersection_update share the same PySetInner::intersection_update, we can do better 😊
|
@youknowone I think these operations are related closely. @Snowapril Thanks for your advice! I'll try to improve it! 👍 |
kth496
force-pushed
the
improve-inplace-op
branch
3 times, most recently
from
2da603f to
b378f64
Compare
kth496
force-pushed
the
improve-inplace-op
branch
from
b378f64 to
2930c84
Compare
|
@youknowone @Snowapril I tried to remove duplicated codes. But there are some different constraints between inplace operation methods. (Please See NOTE below) NOTE:
Same issue on |
youknowone
left a comment
youknowone
left a comment
There was a problem hiding this comment.
About your concern, I think your approach is reasonable enough.
Maybe there can be more room to do better, but current versions looks good enough to handle them in proper cost.
| zelf.inner | ||
| .intersection_update(std::iter::once(set.into_iterable(vm)?), vm)?; | ||
| fn iand(zelf: PyRef<Self>, set: PyObjectRef, vm: &VirtualMachine) -> PyResult<PyRef<Self>> { | ||
| if !set.class().is(vm.ctx.types.set_type) { |
There was a problem hiding this comment.
how about frozenset? if frozenset is allowed, maybe AnySet check?
There was a problem hiding this comment.
yeah, frozen sets are allowed here. Similarly for isub and ixor.
| if let Some(iterable) = arguments.first() { | ||
| if zelf.is(iterable) { | ||
| return Ok(()); | ||
| } | ||
| } |
There was a problem hiding this comment.
I think this check is not only adaptable for len() == 1 case but also do for every argument.
For example, how about s = set(['a']); s.intersection_update(s, s, s, s, s, s)?
| .intersection_update(std::iter::once(set.into_iterable(vm)?), vm)?; | ||
| fn iand(zelf: PyRef<Self>, set: PyObjectRef, vm: &VirtualMachine) -> PyResult<PyRef<Self>> { | ||
| if !set.class().is(vm.ctx.types.set_type) { | ||
| return Err(vm.new_type_error("Type error".to_owned())); |
There was a problem hiding this comment.
Should be "unsupported operand type(s) for +=: 'type1' and 'type2'", similarly for the rest of the type errors.
youknowone
force-pushed
the
improve-inplace-op
branch
from
2930c84 to
0312c8e
Compare
4 participants
Fix #3881