Fix the style for bitflags! #503

TethysSvensson · 2017-01-31T20:12:04Z

Prefer libc_bitflags! over bitflags!. Prefer libc::CONSTANTS over
writing the constant manually.

This makes #501 unnecessary, since upstream now contains the O_TMPFILE constant.

TethysSvensson · 2017-01-31T20:22:33Z

This compiles for me on as many targets as master, though I have not run any tests locally. Can I assume that Travis does a sufficient job for testing?

It does not compile on the following targets (and neither does master):

*-apple-ios
*-linux-android
*-linux-androideabi
*-linux-musleabi
*-linux-musleabihf
*-netbsd
mips*
powerpc*
s390x*

I will try to do follow-up PRs to fix that.

fiveop

Great work. Is there a reason why some of the bitflags! instances were not converted to libc_bitflags!?

TethysSvensson · 2017-02-01T18:34:41Z

Yes, I used libc_bitflags! whenever all of the definitions are currently present in the libc crate under the correct types and used bitflags! in the remaining places.

I also might have missed a few of the definitions, but I think I got most of them.

kamalmarhubi · 2017-02-14T06:08:49Z

This is great!

@idolf

I used libc_bitflags! whenever all of the definitions are currently present in the libc crate under the correct types and used bitflags! in the remaining places.

How are you deciding what the correct types are?

TethysSvensson · 2017-02-14T22:01:12Z

@kamalmarhubi: This PR did not change any of the types. I meant "correct types" as being the same ones as currently used in nix. For instance libc::EPOLLIN is available as a libc::c_int, but nix uses it as a u32.

homu · 2017-02-17T04:32:19Z

☔ The latest upstream changes (presumably #515) made this pull request unmergeable. Please resolve the merge conflicts.

TethysSvensson · 2017-02-17T14:12:57Z

Rebased on top of current master.

homu · 2017-02-17T17:30:18Z

☔ The latest upstream changes (presumably #508) made this pull request unmergeable. Please resolve the merge conflicts.

Prefer libc_bitflags! over bitflags!. Prefer libc::CONSTANTS over writing the constant manually.

TethysSvensson · 2017-02-19T07:44:52Z

Rebased again. Is there any chance that you could get this merged soon, so I won't have to keep rebasing it?

This is not only a style fix, but actually fixes at least one constant (O_TMPFILE) and most likely more.

kamalmarhubi

Only one change required: changing the representation type of EpollFlags.

The rest are just thoughts on more stuff we can do in the same vein after changing libc.

kamalmarhubi · 2017-02-22T16:52:14Z

src/fcntl.rs

-            const O_CLOEXEC   = 0o02000000,
-            const O_SYNC      = 0o04000000,
+            const O_CLOEXEC   = libc::O_CLOEXEC,
+            const O_SYNC      = libc::O_SYNC,
            const O_PATH      = 0o10000000,


not blocking this PR, but if these are missing in libc would you be up for adding them there? Then we could have a followup PR here to use only libc-defined constants.

I agree that it would be a good idea to get more constants upstream and to fix their types. I've already filed rust-lang/libc#503 and rust-lang/libc#504 which is progress towards this goal.

That being said, my main interest in this was to fix the value of O_TMPFILE in a non-hacky way. I might do more at a later stage, but I have less motivation to do so now that my use-case has been fixed.

Oh and rust-lang/libc#506 as well.

Yeah no pressure to do it, and certainly not on this PR. I'll probably do a few myself over the coming days.

kamalmarhubi · 2017-02-22T16:52:44Z

src/fcntl.rs

+            const O_CLOEXEC   = libc::O_CLOEXEC,
+            const O_SYNC      = libc::O_SYNC,
+            const O_NDELAY    = libc::O_NDELAY,
+            const O_FSYNC     = libc::O_FSYNC,
            const O_SHLOCK    = 0x0000080,
            const O_EXLOCK    = 0x0000020,
            const O_DIRECT    = 0x0010000,


same here re non-libc constants & defining them upstream

kamalmarhubi · 2017-02-22T16:53:07Z

src/mount.rs

+        const MS_RELATIME    = libc::MS_RELATIME,
+        const MS_KERNMOUNT   = libc::MS_KERNMOUNT,
+        const MS_I_VERSION   = libc::MS_I_VERSION,
+        const MS_STRICTATIME = libc::MS_STRICTATIME,
        const MS_NOSEC       = 1 << 28,
        const MS_BORN        = 1 << 29,


kamalmarhubi · 2017-02-22T16:53:16Z

src/sys/epoll.rs

+        const EPOLLMSG = libc::EPOLLMSG as u32,
+        const EPOLLERR = libc::EPOLLERR as u32,
+        const EPOLLHUP = libc::EPOLLHUP as u32,
+        const EPOLLRDHUP = libc::EPOLLRDHUP as u32,
        const EPOLLEXCLUSIVE = 1 << 28,


kamalmarhubi

forgot to actually include the comment on the requested change!

kamalmarhubi · 2017-02-22T16:58:18Z

src/sys/epoll.rs

@@ -8,21 +8,21 @@ use ::Error;
 bitflags!(
    #[repr(C)]
    pub flags EpollFlags: u32 {


let's change this to libc::c_int.

kamalmarhubi · 2017-02-22T17:00:56Z

This is basically done modulo changing the type of EpollFlags. Could

kamalmarhubi · 2017-02-22T17:01:10Z

Argh. I am failing at GitHub today...

kamalmarhubi · 2017-02-22T21:45:18Z

@idolf thanks. Not sure if it'll pass or require dropping as u32 on the definitions. Let's find out!

@homu r+

homu · 2017-02-22T21:45:23Z

📌 Commit 716f216 has been approved by kamalmarhubi

kamalmarhubi · 2017-02-22T21:45:24Z

@idolf thanks. Not sure if it'll pass or require dropping as u32 on the definitions. Let's find out!

@homu r+

homu · 2017-02-22T21:45:27Z

💡 This pull request was already approved, no need to approve it again.

There's another pull request that is currently being tested, blocking this pull request: Fix unresolved name error when building with musl. #513

homu · 2017-02-22T21:45:28Z

📌 Commit 716f216 has been approved by kamalmarhubi

homu · 2017-02-22T21:45:29Z

💡 This pull request was already approved, no need to approve it again.

There's another pull request that is currently being tested, blocking this pull request: Fix unresolved name error when building with musl. #513

TethysSvensson · 2017-02-22T21:46:53Z

I'm guessing it probably will.

TethysSvensson · 2017-02-22T21:49:03Z

Yep, it did not build locally. I've pushed a fixed commit now.

TethysSvensson · 2017-02-22T23:48:50Z

Can I tell homu to retry, or do you need privileges for that?

@homu retry

kamalmarhubi · 2017-02-23T00:10:06Z

@homu retry

kamalmarhubi · 2017-02-23T00:11:27Z

@homu r+

homu · 2017-02-23T00:11:28Z

📌 Commit 51b5eac has been approved by kamalmarhubi

Fix the style for bitflags! Prefer `libc_bitflags!` over `bitflags!`. Prefer `libc::CONSTANTS` over writing the constant manually. This makes #501 unnecessary, since upstream now contains the `O_TMPFILE` constant.

homu · 2017-02-23T00:11:43Z

⌛ Testing commit 51b5eac with merge 1fb5fe3...

kamalmarhubi · 2017-02-23T00:13:49Z

Can I tell homu to retry, or do you need privileges for that?

I guess I can't either. I think retry only applies to the same commit.

Anyway, thanks again!

homu · 2017-02-23T04:19:17Z

💥 Test timed out

kamalmarhubi · 2017-02-23T07:33:19Z

@homu retry

Fix the style for bitflags! Prefer `libc_bitflags!` over `bitflags!`. Prefer `libc::CONSTANTS` over writing the constant manually. This makes #501 unnecessary, since upstream now contains the `O_TMPFILE` constant.

homu · 2017-02-23T07:33:33Z

⌛ Testing commit 51b5eac with merge 2bb718c...

homu · 2017-02-23T08:19:03Z

☀️ Test successful - status

fiveop reviewed Feb 1, 2017

View reviewed changes

TethysSvensson force-pushed the bitflags branch from 8d9d072 to 4bb6640 Compare February 17, 2017 14:12

TethysSvensson force-pushed the bitflags branch 2 times, most recently from 0387331 to 3433d9b Compare February 17, 2017 14:17

Mathias Svensson added 2 commits February 19, 2017 08:41

Fix the style for bitflags!

ab453e4

Prefer libc_bitflags! over bitflags!. Prefer libc::CONSTANTS over writing the constant manually.

Update CHANGELOG.md

7435cb6

TethysSvensson force-pushed the bitflags branch from 3433d9b to 7435cb6 Compare February 19, 2017 07:42

kamalmarhubi requested changes Feb 22, 2017

View reviewed changes

kamalmarhubi reviewed Feb 22, 2017

View reviewed changes

kamalmarhubi closed this Feb 22, 2017

kamalmarhubi reopened this Feb 22, 2017

kamalmarhubi approved these changes Feb 22, 2017

View reviewed changes

TethysSvensson force-pushed the bitflags branch from 716f216 to 4447d78 Compare February 22, 2017 21:47

Fix the inner type of EpollFlags

51b5eac

TethysSvensson force-pushed the bitflags branch from 4447d78 to 51b5eac Compare February 22, 2017 21:48

homu merged commit 51b5eac into nix-rust:master Feb 23, 2017

TethysSvensson deleted the bitflags branch February 23, 2017 18:58

Fix the style for bitflags! #503

Fix the style for bitflags! #503

Conversation

TethysSvensson commented Jan 31, 2017

TethysSvensson commented Jan 31, 2017 • edited Loading

fiveop left a comment

Choose a reason for hiding this comment

TethysSvensson commented Feb 1, 2017

kamalmarhubi commented Feb 14, 2017

TethysSvensson commented Feb 14, 2017

homu commented Feb 17, 2017

TethysSvensson commented Feb 17, 2017

homu commented Feb 17, 2017

TethysSvensson commented Feb 19, 2017

kamalmarhubi left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kamalmarhubi left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kamalmarhubi commented Feb 22, 2017

kamalmarhubi commented Feb 22, 2017

kamalmarhubi commented Feb 22, 2017

homu commented Feb 22, 2017

kamalmarhubi commented Feb 22, 2017

homu commented Feb 22, 2017

homu commented Feb 22, 2017

homu commented Feb 22, 2017

TethysSvensson commented Feb 22, 2017

TethysSvensson commented Feb 22, 2017

TethysSvensson commented Feb 22, 2017

kamalmarhubi commented Feb 23, 2017

kamalmarhubi commented Feb 23, 2017

homu commented Feb 23, 2017

homu commented Feb 23, 2017

kamalmarhubi commented Feb 23, 2017 • edited Loading

homu commented Feb 23, 2017

kamalmarhubi commented Feb 23, 2017

homu commented Feb 23, 2017

homu commented Feb 23, 2017

TethysSvensson commented Jan 31, 2017 •

edited

Loading

kamalmarhubi left a comment •

edited

Loading

kamalmarhubi left a comment •

edited

Loading

kamalmarhubi commented Feb 23, 2017 •

edited

Loading