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.

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

Revise pyobject_* macro exports #158

Closed
wants to merge 5 commits into from
Closed

Revise pyobject_* macro exports #158

wants to merge 5 commits into from

Conversation

termoshtt
Copy link

@termoshtt termoshtt commented May 13, 2018

Fix for #155

  • Use expr and path for macro arguments
    • Fix macro call
  • Re-enable fn_must_use to reduce warnings

@@ -1,4 +1,4 @@
#![feature(specialization, proc_macro)]
#![feature(specialization, proc_macro, fn_must_use)]
Copy link
Member

Choose a reason for hiding this comment

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

This might be because you're nightly is an older one. Re-adding this gives a warning on recent nightlies:

warning: this feature has been stable since 1.27.0. Attribute no longer needed
 --> src/lib.rs:1:40
  |
1 | #![feature(specialization, proc_macro, fn_must_use)]
  |                                        ^^^^^^^^^^^
  |
  = note: #[warn(stable_features)] on by default

I've just increased the minimum required version to 1.27.0, so this should be caught by the build script in the future.

@@ -59,10 +59,10 @@ macro_rules! pyobject_native_type_named(
($name: ident) => {
impl $crate::PyNativeType for $name {}

impl $crate::std::convert::AsRef<$crate::PyObjectRef> for $name {
impl ::std::convert::AsRef<$crate::PyObjectRef> for $name {
Copy link
Member

Choose a reason for hiding this comment

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

Does ::std have an advantage over $crate::std?

@konstin
Copy link
Member

konstin commented May 13, 2018

You should also take a look at the travis ci build. It looks like the python 2 module does also need some ffi:: prefixing.

@konstin
Copy link
Member

konstin commented Jul 1, 2018

I'm closing this since the changes have been implemented in #186.

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