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

Get all the tests building and passing! #1189

Merged
merged 1 commit into from
Sep 18, 2020
Merged

Get all the tests building and passing! #1189

merged 1 commit into from
Sep 18, 2020

Conversation

alex
Copy link
Contributor

@alex alex commented Sep 15, 2020

This incorporates #1188, so I'll rebase it once that merges.

@alex alex mentioned this pull request Sep 15, 2020
Closed
@alex alex force-pushed the abi3-tests-compile branch 2 times, most recently from de4e521 to 7920d2f Compare September 15, 2020 12:51
@alex alex changed the title Get all the tests building, everythign except doctests passes! Get all the tests building and passing! Sep 15, 2020
@alex
Copy link
Contributor Author

alex commented Sep 15, 2020

I think I've resolved the doctest issue.

sebpuetz
sebpuetz reviewed Sep 16, 2020
View reviewed changes
Copy link
Contributor

@sebpuetz sebpuetz left a comment

Choose a reason for hiding this comment

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

I took a look at this, I'm not deep into the topic, so most of the comments are superficial or aimed at legibility etc.

pyo3-derive-backend/src/from_pyobject.rs
@@ -249,7 +249,7 @@ impl<'a> Container<'a> {
let self_ty = &self.path;
let mut fields: Punctuated<TokenStream, syn::Token![,]> = Punctuated::new();
for i in 0..len {
fields.push(quote!(slice[#i].extract()?));
fields.push(quote!(s.get_item(#i).extract()?));
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the memory layout different for PyTuples with limited ABI? I know this change here is necessary because as_slice() is no longer available on PyTuple:

pyo3/src/types/tuple.rs

Lines 84 to 93 in 2ec1c3b

#[cfg(not(Py_LIMITED_API))]
pub fn as_slice(&self) -> &[&PyAny] {
// This is safe because &PyAny has the same memory layout as *mut ffi::PyObject,
// and because tuples are immutable.
unsafe {
let ptr = self.as_ptr() as *mut ffi::PyTupleObject;
let slice = slice::from_raw_parts((*ptr).ob_item.as_ptr(), self.len());
&*(slice as *const [*mut ffi::PyObject] as *const [&PyAny])
}
}

It probably won't have a measurable impact anyways, just curious..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They can be, which is the whole point of the limited ABI :-)

@alex
Copy link
Contributor Author

alex commented Sep 16, 2020

Just a note that this PR includes #1188 so it's probably better to review that one first.

@sebpuetz
Copy link
Contributor

I moved the applicable comments to #1188 and deleted them in this PR. There's still a question based on changes in this PR. The actual changes that make the change in this PR necessary happened earlier and are already merged in the abi3 branch, so I think this is the correct place to discuss it.

@alex alex force-pushed the abi3-tests-compile branch from 7920d2f to ba10560 Compare September 16, 2020 12:42
@alex alex marked this pull request as ready for review September 16, 2020 12:42
@alex
Copy link
Contributor Author

alex commented Sep 16, 2020

Ok, rebased and ready to go!

kngwyu
kngwyu reviewed Sep 16, 2020
View reviewed changes
guide/src/class.md
@@ -271,7 +271,7 @@ You can also inherit native types such as `PyDict`, if they implement
However, because of some technical problems, we don't currently provide safe upcasting methods for types
that inherit native types. Even in such cases, you can unsafely get a base class by raw pointer conversion.

```rust
```rust,no_run
Copy link
Member

Choose a reason for hiding this comment

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

What is the problem with this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

subclasses a builtin type which isn't support on abi3 (and I don't know how to make a doc test no_run only in some cfgs)

Copy link
Member

Choose a reason for hiding this comment

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

OK, though I'll have to modify this when it is changed to raise a compile error.

kngwyu
kngwyu approved these changes Sep 18, 2020
View reviewed changes
Copy link
Member

@kngwyu kngwyu left a comment

Choose a reason for hiding this comment

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

Thanks!

@kngwyu kngwyu merged commit c87a59c into PyO3:abi3 Sep 18, 2020
@alex alex deleted the abi3-tests-compile branch September 18, 2020 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sebpuetz sebpuetz sebpuetz left review comments

@kngwyu kngwyu kngwyu 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.
3 participants
@alex @sebpuetz @kngwyu