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

Allow doc attributes in ioctl #661

Merged
merged 1 commit into from
Jul 23, 2017

Conversation

roblabla
Copy link
Contributor

@roblabla roblabla commented Jul 8, 2017

fixes #571 . Note that this is a breaking change because it also changes

ioctl!(some_name with 12);

to

ioctl!(bad some_name with 12);

This is to work around a bug in the rust compiler whereby rules around matching idents are overly strict. See rust-lang/rust#24189

It doesn't break anything else though.

@@ -97,6 +97,16 @@
//! Most `ioctl`s have no or little documentation. You'll need to scrounge through
//! the source to figure out what they do and how they should be used.
//!
//! How do I document the generated functions?
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's change this to "Documenting generated ioctl functions". I'd also like to request that you add a rationale why this was added (that generated ioctls are public and should have docs and that many crates error on missing documentation).

@@ -1,7 +1,7 @@
#![allow(dead_code)]

// Simple tests to ensure macro generated fns compile
ioctl!(do_bad with 0x1234);
ioctl!(bad do_bad with 0x1234);
Copy link
Contributor

Choose a reason for hiding this comment

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

We also need a test here that uses a comment with all of the various forms of the ioctl! macro given that this is the functionality you're really adding here.

@Susurrus
Copy link
Contributor

Susurrus commented Jul 9, 2017

I'd also like to see this refactored into 2 commits. First fix the bad version of the macro and update the tests in one commit. Then change the macros to support doccomments, update the docs, and add tests for the documented version of all forms of the ioctl! macro. I prefer to keep commits to a single issue as it's easier to read through version history if you do that. In this case since it will break code, I'd like a nice clear commit that we can point to for someone what shows the exact change clearly and how they can update their code.

@Susurrus
Copy link
Contributor

I did a bunch of minor cleanup in ioctl while I was there to improve the docs. I'd like to have you rebase this PR once we get that merged so this just adds the comments and tests.

@roblabla
Copy link
Contributor Author

I rebased on top of master, and made some of the requested modifications. Is this good enough ?

@Susurrus
Copy link
Contributor

This looks good. Can you squash this into a single commit? Then we can merge it.

@roblabla
Copy link
Contributor Author

Done.

@asomers
Copy link
Member

asomers commented Jul 23, 2017

bors r+ susurrus

bors bot added a commit that referenced this pull request Jul 23, 2017
661: Allow doc attributes in ioctl r=asomers

fixes #571 . Note that this is a breaking change because it also changes 

```
ioctl!(some_name with 12);
```

to

```
ioctl!(bad some_name with 12);
```

This is to work around a bug in the rust compiler whereby rules around matching idents are overly strict. See rust-lang/rust#24189

It doesn't break anything else though.
@bors
Copy link
Contributor

bors bot commented Jul 23, 2017

@bors bors bot merged commit 4b8a661 into nix-rust:master Jul 23, 2017
@Susurrus
Copy link
Contributor

I missed this on the original PR, but this should also have added a CHANGELOG entry. @roblabla Could you add one to the Added section for this?

asomers added a commit to asomers/nix that referenced this pull request Jul 24, 2017
bors bot added a commit that referenced this pull request Jul 24, 2017
704: Add CHANGELOG entry for PR #661 r=Susurrus

cc @roblabla
marmistrz pushed a commit to marmistrz/nix that referenced this pull request Jul 25, 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.

Use of ioctl!() in a crate with [warn(missing_docs)]
3 participants