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

Ioctl cleanup #185

Closed
wants to merge 4 commits into from
Closed

Ioctl cleanup #185

wants to merge 4 commits into from

Conversation

posborne
Copy link
Member

@posborne posborne commented Sep 1, 2015

This set of changes addresses the inability to expose a public interface by making generating functions for ioctl! public. It also includes documentation improvements and some code cleanup.

I have some other ideas for ways to enhance the ioctl! macros to support other common use cases better but am holding off on them for the moment.

@carllerche
Copy link
Contributor

This PR doesn't seem to be passing CI atm.

Readability was unecessarily impaired via a myriad of attributes to
hide constants from the documentation.  If these attributes are exposed
publically, including them in the documentation makes sense.
Consumers of the API may control visibility by means of a module.  The
following is a useful pattern that may be used by implementors (here for
a couple of i2cdev ioctl definitions):

    mod ioctl {
        ioctl!(bad set_i2c_slave_address with super::I2C_SLAVE);
        ioctl!(bad i2c_smbus with super::I2C_SMBUS);
    }

This resolves nix-rust#184.
@posborne
Copy link
Member Author

posborne commented Sep 5, 2015

The doctest was missing a cfg guard as it will only pass under Linux. That has been added now (and I rebased on master), so it should pass now.

The biggest change here is making ioctl! generate public functions now as was brought up in #184. @jmesmon does bring up a good point that in most cases you will not really want to directly export unsafe code. I think the public probably makes sense as then you have the option to export or not via the use of a module namespace as is shown in the example that is added.

@posborne
Copy link
Member Author

@carllerche, this one should be ready for review again.

@carllerche
Copy link
Contributor

looks good to me! I will merge.

@carllerche
Copy link
Contributor

Merged: 0fd0337

@carllerche carllerche closed this Sep 15, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants