Signal enum #362

fiveop · 2016-04-25T16:00:12Z

This is work in progress. I post this pull request as a request for discussion. I mark the lines I have doubts or ideas about.

Ignore the first commit. It is the same as PR #361, which I wanted to base this change off.

fiveop · 2016-04-25T16:01:24Z

src/sys/signal.rs

-pub const SIGEMT: libc::c_int = 7;
+#[derive(Clone, Copy, Debug, Eq, PartialEq)]
+pub enum Signal {
+    SIGHUP = libc::SIGHUP as isize,


I would have prefered #[repr(libc::c_int)] but that does not seem to be possible. Maybe that's worth an RFC for the language.

I came across the same issue in #364 though I decided to go with #[repr(i32)] since the type of the constants in libc crate (c_int is i32 unconditionally), and cast to c_int at use. Thinking about it a bit more, I'm not super happy about it, because if libc ever makes c_int conditional, we'll have trouble.

And yeah, I think it would be good if the language supported type aliases in #[repr] attributes.

I worte a draft for an RFC regarding this: https://github.com/fiveop/rfcs/blob/master/text/0000-type-aliases-in-enumeration-repr-attritubes.md

fiveop · 2016-04-30T13:29:57Z

@kamalmarhubi I commented on yours. Now, it is your turn ;)

kamalmarhubi · 2016-04-30T13:57:28Z

src/sys/signal.rs

+}
+
+// TODO: NOT PUBLIC :/
+pub type SigNum = libc::c_int;


I wonder if we could instead define From<T> for Signal for various integral types? Our code and user code could then write Signal::from(9) for example.

Signal::from(9) fits to the signal_from method above.In that case, I agree. However, I do not want to provide users access to it (as mentioned above).

kamalmarhubi · 2016-04-30T14:08:43Z

Not for this PR: one thing from looking more closely at this module than I have before: do the thread functions really belong as impl SigSet? I think they would more belong on the thread object via some extensions trait.

fiveop · 2016-05-05T08:07:36Z

I worked on this PR again, after adapting code that uses the sys::signal module.

Since I have to write functions with C-ABI as signal handlers, which have to take SigNum instead of Signal because of type safety, I noticed that it does make sense to export the conversion function from SigNum to Signal, so the From trait fits the bill perfectly.

I am now pretty happy with the state of the PR. Any more remarks?

kamalmarhubi · 2016-05-05T18:03:43Z

src/sys/signal.rs

+}
+
+
+pub type SigNum = libc::c_int;


one thing I'm wondering is what to do we get out of an extra indirection here? posix specifies they be int:

The <signal.h> header shall define the following macros that are used to refer to the signals that occur in the system. Signals defined here begin with the letters SIG followed by an uppercase letter. The macros shall expand to positive integer constant expressions with type int and distinct values. The value 0 is reserved for use as the null signal (see kill()). Additional implementation-defined signals may occur in the system.

—http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/signal.h.html

It signals that we do not only want any int but one out of the range for signals.

but a type alias doesn't do anything to help with that?

oh as documentation somehow, I see. hmmmm...

I admit, that one can argue, that it hides the fact that it's unsafe.

I'm now wondering if we should be using From for this. From docs:

Note: this trait must not fail. If the conversion can fail, use a dedicated method which return an Option<T> or a Result<T, E>.

―http://doc.rust-lang.org/std/convert/trait.From.html

The TryFrom/TryInto RFC was accepted 2 days ago.

I will provide our own implementation and add a comment as well as issue regarding the new traits, once we merge this.

@utkarshkukreti amazing! I knew it was being worked on, hadn't realized it was already written and merged.

fiveop · 2016-05-05T18:33:19Z

I changed how the iterator is created.

fiveop · 2016-05-06T18:38:09Z

I worked on this again. Two things changed:

The From implementation was replaced by the signal_from function because From may not fail. A comment regarding the comming trait TryFrom was added.
Signal::iterator now returns our own Iterator struct, so that changes in implementation won't break the interface.

kamalmarhubi · 2016-05-06T18:42:01Z

The From implementation was replaced by the signal_from function because From may not fail. A comment regarding the comming trait TryFrom was added.

Given this change, the motivation for having SigNum in our public API seems a bit weaker. Do you think we can remove it now?

kamalmarhubi · 2016-05-06T18:42:43Z

Also @fiveop thanks for being so patient through all the comments! I'm reasonably sure this is the most commented PR yet. (GitHub's sort doesn't seem to work properly, so am not completely sure.)

homu · 2016-05-06T20:39:14Z

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

fiveop · 2016-05-07T16:42:54Z

I removed SigNum. With signal_from failing properly it makes sense.

Any more concerns?

kamalmarhubi · 2016-05-17T22:16:55Z

src/lib.rs

@@ -8,7 +8,6 @@
 // latest bitflags triggers a rustc bug with cross-crate macro expansions causing dead_code
 // warnings even though the macro expands into something with allow(dead_code)
 #![allow(dead_code)]
-#![deny(warnings)]


is there a reason this was removed in this PR?

Probably, because it wouldn't compile otherwise on whatever was the current nightly then. I'll recheck it, when I look at it again.

No worries, was just curious.

kamalmarhubi · 2016-05-17T22:27:44Z

Ok I think I will promise not to make any more requests after these three I just made!

fiveop · 2016-06-03T15:07:28Z

I only see two messages? What's the third concern?

kamalmarhubi · 2016-06-04T09:08:37Z

src/sys/signal.rs

+
+pub const SIGIOT : Signal = SIGABRT;
+pub const SIGPOLL : Signal = SIGIO;
+pub const SIGUNUSED : Signal = SIGSYS;


are these aliases cross-platform?

kamalmarhubi · 2016-06-04T09:11:16Z

@fiveop

I only see two messages? What's the third concern?

Honestly no idea! Maybe I can't count past one correctly. Anyway, feel free to r=me once signal_from changes. Thanks so much for doing this!

homu · 2016-06-11T09:17:09Z

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

fiveop · 2016-06-11T10:38:57Z

finally done?

kamalmarhubi · 2016-06-12T08:44:00Z

@homu r+

🎉

homu · 2016-06-12T08:44:01Z

📌 Commit 98063a7 has been approved by kamalmarhubi

Signal enum This is work in progress. I post this pull request as a request for discussion. I mark the lines I have doubts or ideas about. Ignore the first commit. It is the same as PR #361, which I wanted to base this change off.

homu · 2016-06-12T08:44:13Z

⌛ Testing commit 98063a7 with merge c418266...

kamalmarhubi · 2016-06-12T08:45:05Z

@homu r-

kamalmarhubi · 2016-06-12T08:45:15Z

Some failures on Apple?

utkarshkukreti · 2016-06-12T08:50:09Z

src/sys/signal.rs

+    #[cfg(target_os = "macos")]
+    SIGEMT = libc::SIGEMT,
+    #[cfg(target_os = "macos")]
+    SIGINFO = libc::SIGEMT,


@kamalmarhubi typo here?

homu · 2016-06-12T09:01:47Z

💔 Test failed - status

fiveop · 2016-06-26T15:33:37Z

this should be good to merge now as well (finally! :))

posborne · 2016-06-27T22:58:15Z

Although @kamalmarhubi did most of the reviewing before, I looked things over again and it looks good to me. @homu r+

homu · 2016-06-27T22:58:16Z

📌 Commit 5c7b3c1 has been approved by posborne

homu · 2016-06-27T22:58:30Z

⚡ Test exempted - status

Signal enum This is work in progress. I post this pull request as a request for discussion. I mark the lines I have doubts or ideas about. Ignore the first commit. It is the same as PR #361, which I wanted to base this change off.

fiveop reviewed Apr 25, 2016
View reviewed changes

kamalmarhubi reviewed Apr 30, 2016
View reviewed changes

kamalmarhubi mentioned this pull request Apr 30, 2016

Add getrlimit(2) and setrlimit(2) #364

Closed

fiveop mentioned this pull request May 5, 2016

Allow type aliases in enumeration repr attributes. rust-lang/rfcs#1605

Closed

kamalmarhubi reviewed May 5, 2016
View reviewed changes

kamalmarhubi reviewed May 17, 2016
View reviewed changes

kamalmarhubi reviewed Jun 4, 2016
View reviewed changes

utkarshkukreti reviewed Jun 12, 2016
View reviewed changes

Convert signal constants to enumeration.

5c7b3c1

homu merged commit 5c7b3c1 into nix-rust:master Jun 27, 2016

homu mentioned this pull request Jun 27, 2016

wait: support ptrace events for Linux #273

Closed

fiveop deleted the signal_enum branch June 29, 2016 17:06

Signal enum #362

Signal enum #362

Conversation

fiveop commented Apr 25, 2016

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

fiveop commented Apr 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kamalmarhubi commented Apr 30, 2016

fiveop commented May 5, 2016

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 May 6, 2016 • 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

fiveop commented May 5, 2016

fiveop commented May 6, 2016

kamalmarhubi commented May 6, 2016

kamalmarhubi commented May 6, 2016 • edited Loading

homu commented May 6, 2016

fiveop commented May 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kamalmarhubi commented May 17, 2016

fiveop commented Jun 3, 2016

Choose a reason for hiding this comment

kamalmarhubi commented Jun 4, 2016

homu commented Jun 11, 2016

fiveop commented Jun 11, 2016

kamalmarhubi commented Jun 12, 2016

homu commented Jun 12, 2016

homu commented Jun 12, 2016

kamalmarhubi commented Jun 12, 2016

kamalmarhubi commented Jun 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

homu commented Jun 12, 2016

fiveop commented Jun 26, 2016

posborne commented Jun 27, 2016

homu commented Jun 27, 2016

homu commented Jun 27, 2016

kamalmarhubi May 6, 2016 •

edited

Loading

kamalmarhubi commented May 6, 2016 •

edited

Loading