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

enum: Replacing current type conversion by enums #9

Closed
wants to merge 1 commit into from

Conversation

farodin91
Copy link

@farodin91 farodin91 commented Jun 7, 2017

Fixes #7
@sahid What do you think?

Copy link
Owner

@sahid sahid left a comment

Choose a reason for hiding this comment

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

Hum.. I'm not totally convinced by this, is there any document or example which are using such practice for ffi?

src/connect.rs Outdated
pub const VIR_CRED_EXTERNAL: ConnectCredentialType = 9;
virt_enum! {
ConnectCredentialType {
Username -> 1,
Copy link
Owner

Choose a reason for hiding this comment

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

With your change it will be still possible to document the fields? something like:

virt_enum! {
  ConnectCredentialType {
    /// Requests username blabla....
    Username -> 1,
    ...
}

Copy link
Author

Choose a reason for hiding this comment

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

I need to change something, using meta attr.

@farodin91
Copy link
Author

I didn't find a good example. You can use enums easly in match. For me it looks better. I'm also not finished.

@farodin91
Copy link
Author

This version force comments, but only support a single command. The reason is a bug in rust rust-lang/rust#24827.

pub const VIR_CRED_NOECHOPROMPT: ConnectCredentialType = 7;
pub const VIR_CRED_REALM: ConnectCredentialType = 8;
pub const VIR_CRED_EXTERNAL: ConnectCredentialType = 9;
virt_enum! {
Copy link
Owner

Choose a reason for hiding this comment

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

It's a very nice work but I dont think we should use a macro. We don't have so much constants to convert so we should implement them correctly using an enum.

Then about to do the conversion I'm thinkin about using Procedural Macros so perhaps at the end to have something like:

#[Derive(ConvertToC)]
pub enum Something {
  Value1 = 1,
  Value2 = 2,
  ...
}

Also it's very nice to have started documenting the fields, if you really want do that, the best is to reuse what we have in the header of libvirt, like: http://libvirt.org/git/?p=libvirt.git;a=blob;f=include/libvirt/libvirt-domain.h;h=45f939a8cc37e8e29bea13316a222338b53774fe;hb=HEAD#l291

Also a last question what about to keep the name in uppercase?

#[Derive(ConvertToC)]
pub enum Something {
  VALUE1 = 1,
  VALUE1 = 2,
  ...
}

Copy link
Author

Choose a reason for hiding this comment

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

This should also work.

#[repr(C)]
pub enum Test {
  Value = 1
}

Copy link
Owner

Choose a reason for hiding this comment

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

This is how I would see the thing:

https://github.com/sahid/libvirt-rs/blob/poc/enum/src/error.rs#L45

What do you think?

Copy link
Author

Choose a reason for hiding this comment

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

This would be great.

Copy link
Author

@farodin91 farodin91 Jun 8, 2017

Choose a reason for hiding this comment

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

Combined solution would be wrapping these into an macro to auto create From traits until derive is implemented.

impl_enum!{
  #[derive(Debug, PartialEq)]
  pub enum ErrorLevel {
    /// No error
    None = 0,
    /// A simple warning.
    Warning = 1,
    /// An error.
    Error = 2,
  }
}

Normally enums in Rust are CamelCase.

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Owner

@sahid sahid Jun 9, 2017

Choose a reason for hiding this comment

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

My thinking is we should do it by procedural macro and so use derive or we should do it by hands (it's not all the constants that needs to be converted from).

Copy link
Owner

Choose a reason for hiding this comment

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

@farodin91 please let me know if you still want to continue to work on this issue, if not I could take the lead :)

Thanks

Copy link
Author

Choose a reason for hiding this comment

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

You could take it.

@farodin91 farodin91 closed this Jun 19, 2017
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