Skip to content

Implement range.count#381

Merged
windelbouwman merged 4 commits into
RustPython:masterfrom
nficca:implement-range-count
Feb 10, 2019
Merged

Implement range.count#381
windelbouwman merged 4 commits into
RustPython:masterfrom
nficca:implement-range-count

Conversation

@nficca
Copy link
Copy Markdown
Contributor

@nficca nficca commented Feb 8, 2019

Implements count for #293.

Comment thread vm/src/obj/objrange.rs Outdated
);

if let PyObjectPayload::Range { ref range } = zelf.borrow().payload {
match needle.borrow().payload {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Instead of using the PyObjectPayload here, use the vm.isinstance type check to determine int type, and objint::get_value to retrieve the BigInt value.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This works for the argument, but won't I still eventually need to use PyObjectPayload to pull out the actual RangeType? Apologies if I misunderstood you, I'm not entirely familiar with this project.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No worries, thank you for your help! The idea is to use PyObjectPayload as little as possible. The objrange.rs file should contain a method get_value which will retrieve the payload for you. This saves a lot of duplicate code for borrow and matching on the payload type. Take a look at the objint::getvalue method for example. It returns you the BigInt contents of the integer. The same should be there for the range type.

Hope this helps!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Btw, the problem with checking strictly on the payload is that this will eventually fail with subclasses. The isinstance check is more thorough and works for subclasses as well!

- Refactor range_count to use get_value and isinstance for pulling out the range and argument respectively
- Use get_value in all appropriate spots
@nficca nficca force-pushed the implement-range-count branch from 2139a33 to 0cb661f Compare February 9, 2019 21:54
@nficca
Copy link
Copy Markdown
Contributor Author

nficca commented Feb 9, 2019

@windelbouwman I didn't find the objrange::get_value method you mentioned so I've added it myself. I make use of it and objtype::getinstance as you suggested in range_count; I think this is what you meant?

Also, I modified a bunch of the other methods in the file to make use of objrange::get_value as well. Maybe this is beyond the scope of this PR... If so I can move those changes to a separate PR.

@windelbouwman
Copy link
Copy Markdown
Contributor

This is exactly what I meant. Great job!

@windelbouwman windelbouwman merged commit fc38d55 into RustPython:master Feb 10, 2019
@windelbouwman windelbouwman mentioned this pull request Feb 10, 2019
@nficca nficca deleted the implement-range-count branch February 10, 2019 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants