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.

Sign up for GitHub

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

Jump to bottom

Make __r*__ methods work with operators #839

Merged
merged 7 commits into from
Mar 30, 2020
Merged

Make __r*__ methods work with operators #839

merged 7 commits into from
Mar 30, 2020

Conversation

kngwyu
Copy link
Member

@kngwyu kngwyu commented Mar 28, 2020

Resolves #838.
This PR uses __radd__ methods as reversed fallback methods for __add__ or so.
This is the same as what cpython does and I'm sure it's a correct strategy, but, apparently, this implementation is dirty and has lots of templates 🙄
Need to simplify in the future.

@kngwyu kngwyu changed the title Radd fix Make __r*__ methods work with operators Mar 28, 2020
davidhewitt
davidhewitt reviewed Mar 28, 2020
View reviewed changes
tests/test_arithmetics.rs Outdated
Ok(format!("{:?} | RA", other))
}

fn __rpow__(&self, other: &PyAny, _module: &PyAny) -> PyResult<String> {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
fn __rpow__(&self, other: &PyAny, _module: &PyAny) -> PyResult<String> {
fn __rpow__(&self, other: &PyAny, _mod: &PyAny) -> PyResult<String> {

pyo3-derive-backend/src/defs.rs Show resolved Hide resolved
src/class/number.rs Show resolved Hide resolved
src/class/number.rs Show resolved Hide resolved
davidhewitt
davidhewitt reviewed Mar 28, 2020
View reviewed changes
tests/test_arithmetics.rs Outdated
let py = gil.python();

let c = PyCell::new(py, LhsAndRhsArithmetic {}).unwrap();
py_run!(py, c, "assert c.__radd__(1) == '1 + BA'");
Copy link
Member

Choose a reason for hiding this comment

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

So if I understand this test, it means that it's never possible to call fn __radd__ ?

(fn __radd__ has RA in output, this has BA.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, this can be a bug.
I'll check it tommorow.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed by using METH_COEXIST

@davidhewitt
Copy link
Member

davidhewitt commented Mar 29, 2020 •
edited
Loading

👍 nicely figured out! Looks like METH_COEXIST should be a performance improvement too, from the python docs.

@kngwyu
Copy link
Member Author

kngwyu commented Mar 29, 2020

It looks like we can't use COEXIST for __rpow__ since it can take 3rd argument...

@davidhewitt
Copy link
Member

davidhewitt commented Mar 29, 2020 •
edited
Loading

It looks like we can't use COEXIST for __rpow__ since it can take 3rd argument...

I'm not sure that's true... isn't the issue that __pow__ and __rpow__'s modulo argument should be Option<Self::Modulo>?

e.g. the supported forms should be

# 2-argument forms
pow(c, 1) -> c.__pow__(1)
pow(1, c) ->  c.__rpow__(1)

# 3-argument form
pow(c, 1, mod) -> c.__pow__(1, mod)

According to the note here, pow(1, c, mod) will not call c.__rpow__.

@kngwyu
Copy link
Member Author

kngwyu commented Mar 29, 2020

I'm not sure that's true... isn't the issue that pow and rpow's modulo argument should be OptionSelf::Modulo?

👍
Changed to do so.

@kngwyu
Copy link
Member Author

kngwyu commented Mar 29, 2020

Sign, coexist + round cause segfault that I can't reproduce in my local machine... 🤢

@davidhewitt
Copy link
Member

Sign, coexist + round cause segfault that I can't reproduce in my local machine... 🤢

😢 I'll try later on mine and let you know if I have any luck!

@kngwyu
Copy link
Member Author

kngwyu commented Mar 29, 2020

@davidhewitt
Simply I think we shouldn't use METH_COEXIST for __round__ since the builtin round can pass some arguments.

@davidhewitt
Copy link
Member

@davidhewitt
Simply I think we shouldn't use METH_COEXIST for __round__ since the builtin round can pass some arguments.

Ok. I guess also there is no need for METH_COEXIST with __round__ because there is no such thing as __rround__ 😄

@davidhewitt
Copy link
Member

Actions CI failure this time was the usual datetime fail, I'm rerunning jobs...

@kngwyu
Copy link
Member Author

kngwyu commented Mar 30, 2020

@davidhewitt
Thank you for fixing clippy warnings!

@kngwyu kngwyu merged commit 85de698 into PyO3:master Mar 30, 2020
@davidhewitt davidhewitt mentioned this pull request Mar 30, 2020
Closed
@kngwyu kngwyu mentioned this pull request Apr 1, 2020
Closed
@davidhewitt davidhewitt mentioned this pull request Apr 3, 2020
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidhewitt davidhewitt davidhewitt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Binary operators don't work for __r*__ methods
2 participants
@kngwyu @davidhewitt