GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Fix some set operation to compare self with args by kth496 · Pull Request #3912 · RustPython/RustPython

Conversation

@kth496

@kth496 kth496 commented Jul 18, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Fix #3881

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@kth496
kth496 force-pushed the improve-inplace-op branch from d5e969b to ada0063 Compare July 18, 2022 01:03
@youknowone youknowone added the z-ca-2022 Tag to track contrubution-academy 2022 label Jul 18, 2022
@youknowone

youknowone commented Jul 18, 2022

Copy link
Copy Markdown
Member

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Snowapril
Snowapril reviewed Jul 18, 2022
View reviewed changes
Comment thread 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() {

@Snowapril Snowapril Jul 18, 2022

Copy link
Copy Markdown
Contributor

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.

Suggested change
if zelf.get_id() == set.get_id() {
if zelf.is(&set) {

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
youknowone and kth496 reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
Snowapril
Snowapril reviewed Jul 18, 2022
View reviewed changes
Comment thread vm/src/builtins/set.rs Outdated
Comment on lines +662 to +707
if zelf.get_id() == set.get_id() {
return Ok(zelf);
}

@Snowapril Snowapril Jul 18, 2022

Copy link
Copy Markdown
Contributor

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.

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 😊

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
youknowone and kth496 reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@kth496

kth496 commented Jul 18, 2022

Copy link
Copy Markdown
Contributor Author

@youknowone I think these operations are related closely.
So some of the other implementation will be updated in this PR! 😃

@Snowapril Thanks for your advice! I'll try to improve it! 👍
youknowone and Snowapril reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@kth496
kth496 marked this pull request as draft July 19, 2022 14:16
@kth496
kth496 force-pushed the improve-inplace-op branch 3 times, most recently from 2da603f to b378f64 Compare July 19, 2022 17:11
@kth496
kth496 marked this pull request as ready for review July 20, 2022 12:12
@kth496
kth496 force-pushed the improve-inplace-op branch from b378f64 to 2930c84 Compare July 20, 2022 12:21
@kth496

kth496 commented Jul 20, 2022

Copy link
Copy Markdown
Contributor Author

@youknowone @Snowapril
All bugs described in the issue have been fixed. But I think there's another way to do this better. I'm not good at Rust. Could you please review the code?

I tried to remove duplicated codes. But there are some different constraints between inplace operation methods. (Please See NOTE below)

NOTE:

  1. iand

    • Only one argument with AnySet type. Need to check the type of argument.

  2. intersection_update

    • Variable arguments are allowed. (even 0)
    • All Iterable Types are allowed. (include AnySet)

Same issue on isub difference_update ixor ...
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
youknowone
youknowone requested changes Jul 21, 2022
View reviewed changes

@youknowone youknowone left a comment

Copy link
Copy Markdown
Member

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
kth496 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread vm/src/builtins/set.rs
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) {

@youknowone youknowone Jul 21, 2022

Copy link
Copy Markdown
Member

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.

how about frozenset? if frozenset is allowed, maybe AnySet check?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@DimitrisJim DimitrisJim Nov 7, 2022

Copy link
Copy Markdown
Member

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.

yeah, frozen sets are allowed here. Similarly for isub and ixor.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread vm/src/builtins/set.rs
Comment on lines +661 to +692
if let Some(iterable) = arguments.first() {
if zelf.is(iterable) {
return Ok(());
}
}

@youknowone youknowone Jul 21, 2022

Copy link
Copy Markdown
Member

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 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)?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
DimitrisJim reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@tgsong827 tgsong827 mentioned this pull request Aug 2, 2022
&= and -= operator between Set types do not work correctly. #3992
Closed
DimitrisJim
DimitrisJim reviewed Nov 7, 2022
View reviewed changes
Comment thread vm/src/builtins/set.rs
.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()));

@DimitrisJim DimitrisJim Nov 7, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

Should be "unsupported operand type(s) for +=: 'type1' and 'type2'", similarly for the rest of the type errors.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
youknowone reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@DimitrisJim DimitrisJim mentioned this pull request Jan 6, 2023
Fix set bugs with &= and -= #4424
Closed
kth496 added 2 commits February 20, 2023 15:35
@kth496 @youknowone
Fix set inplace operation bug
6a4b607
@kth496 @youknowone
Fix unexpected success test
0312c8e
@youknowone
youknowone force-pushed the improve-inplace-op branch from 2930c84 to 0312c8e Compare February 20, 2023 06:35
@youknowone youknowone closed this in #6661 Jan 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DimitrisJim DimitrisJim DimitrisJim left review comments
@youknowone youknowone youknowone requested changes
+1 more reviewer
@Snowapril Snowapril Snowapril left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

z-ca-2022 Tag to track contrubution-academy 2022

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

set intersection_update, difference_update, symmetric_difference_update should check self is equal to given arg

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@kth496 @youknowone @DimitrisJim @Snowapril
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.