Add From and TryFrom impls in libc_enum macro #1088
Conversation
src/macros.rs
Outdated
| attrs: $attrs, | ||
| entries: $entries, | ||
| } | ||
| @(pub) |
There was a problem hiding this comment.
What is the function of the @ ?
There was a problem hiding this comment.
Oh, it's just denoting that this is an internal rule.
| $(#[$enum_attr:meta])* | ||
| enum $enum:ident : $prim:ty { | ||
| $( | ||
| $(#[doc = $var_doc:tt])* |
There was a problem hiding this comment.
Doc strings and cfg attributes are both different kinds of attributes. Could you simplify the macro by combining these two lines?
There was a problem hiding this comment.
The compiler emits a warning for "unused doc comment" when it sees a doc comment annotating a match arm, so I match them separately here so that I can use the cfg attributes on their own later.
| $( | ||
| $(#[doc = $var_doc])* | ||
| $(#[cfg($var_cfg)])* | ||
| $entry = ::libc::$entry as isize |
There was a problem hiding this comment.
why cast to isize? It seems like $prim would be more appropriate.
There was a problem hiding this comment.
isize is the type that rust expects here. I tried using $prim, but that requires adding a #[repr($prim)] attribute to the enum, which I couldn't get to work for some reason I unfortunately can't remember.
There was a problem hiding this comment.
Given the way that these types will be used, I think that #[repr($prim)] would be better. Could you try again to make it work? Show me the error if you have trouble.
There was a problem hiding this comment.
The problem is that in some places we're using a custom type as $prim, and it turns out the repr attribute only works with simple unsigned or signed integer types u* and i* (see the Rustonomicon).
The reason we need to use a custom type is that in some cases the underlying type depends on the target machine. I'm not sure how we could get around that with the enum Signal : c_int syntax I set up here.
Given the way that these types will be used
Can you share an example where the repr makes a difference?
| $(#[cfg($var_cfg)])* | ||
| ::libc::$entry => Ok($enum::$entry), | ||
| )* | ||
| // don't think this Error is the correct one |
There was a problem hiding this comment.
I think that's a fine choice of error.
There was a problem hiding this comment.
The reason I doubt is that Error::Sys(Errno::EINVAL) probably makes one think this is a "system error" that was picked up from errno.
How about adding an Error::InvalidConversion?
There was a problem hiding this comment.
Yeah, I understand. Error::InvalidConversion would work, too.
|
Fixed some of the review items and rebased on master. |
|
It looks like your build is failing on our MSRV, which is Rust 1.25.0. could you please fix it? |
This PR adds
FromandTryFromimpls inlibc_enum. The macro was rewritten from scratch, and I think it's considerably simpler now. The syntax changed slightly in order to add the "base" type for which it'll generate conversion impls. It looks more like bitflags now.To support Rust versions where
TryFromisn't stable and keep the current MSRV, there is an inherenttry_frommethod. I had to add a build dependency on theversion_checkcrate to support conditionally compiling the trait impl.When
try_fromfails it returnsError::invalid_argument(). I copied this over from an existingSignal::from_c_intfunction, but I'm not sure it's the right error to use. Should we add a newErrorvariant?