Add current_pid function by tmccombs · Pull Request #45059 · rust-lang/rust

tmccombs · 2017-10-06T05:50:34Z

rust-highfive · 2017-10-06T05:50:47Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @BurntSushi (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

Fixesrust-lang#44971

cuviper · 2017-10-06T16:30:31Z

+/// Basic usage:
+///
+/// ```no_run
+/// use std::process:current_pid;


Missing a colon in process::current_pid.

cuviper · 2017-10-06T16:33:27Z

 }
+
+pub fn getpid() -> u32 {
+ syscall::getpid().unwrap() as u32


pid_t is a usize on redox, so this may lose bits.

Does Redox really allow PIDs greater than 4 billion-ish though?

If it does, we'll need to reconsider the current Child::id method.

I made it a u32 to be consistent with Child::id.

cuviper · 2017-10-06T16:34:59Z

 }
+
+pub fn getpid() -> u32 {
+ unsafe { libc::getpid() as u32 }


pid_t is technically signed by standard, defined as i32 on all of libc's unix platforms.

Yes, but getpid() is guaranteed to return a non-negative number, and so u32 is valid here. The signed-ness represents process groups, not IDs.

cuviper · 2017-10-06T16:35:41Z

+///
+///
+#[unstable(feature = "getpid", issue = "44971", reason = "recently added")]
+pub fn current_pid() -> u32 {


I think i64 or u64 would be more prudent.

We already use u32 elsewhere: https://doc.rust-lang.org/std/process/struct.Child.html#method.id

Yep, that's why I chose u32. Although I think usize or u64 would probably make more sense for both.

sfackler · 2017-10-07T17:50:17Z

I think I might lean towards current_id rather than current_pid since we're already in the process module?

sfackler · 2017-10-07T17:50:32Z

@rfcbot fcp merge

rfcbot · 2017-10-07T18:06:34Z

Team member @sfackler has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

alexcrichton · 2017-10-08T02:21:15Z

Looks good to me! I might throw out the names id or current_id, however to jive better with Child::id (we haven't used "pid" yet)

carols10cents · 2017-10-16T15:46:29Z

ping @BurntSushi and @aturon for your ticky boxes here!

kennytm · 2017-10-25T14:03:49Z

Ping @BurntSushi, @aturon again — ticky boxes~

aturon · 2017-10-25T14:42:25Z

Checking off for @BurntSushi as well -- because this is an unstable function, full FCP isn't really needed.

rfcbot · 2017-10-25T15:36:17Z

🔔 This is now entering its final comment period, as per the review above. 🔔

alexcrichton · 2017-10-25T16:06:55Z

@bors: r+

bors · 2017-10-25T16:06:56Z

📌 Commit 29b319b has been approved by alexcrichton

Add current_pid function Fixesrust-lang#44971

Rollup of 7 pull requests - Successful merges: #45059, #45212, #45398, #45483, #45496, #45508, #45526 - Failed merges:

bors · 2017-10-26T00:58:42Z

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

bors · 2017-10-26T00:58:43Z

⌛ Testing commit 29b319b with merge e847f30...

rust-highfive assigned BurntSushi Oct 6, 2017

tmccombs force-pushed the pid branch from f02f373 to fdbf940Compare October 6, 2017 06:07

Add current_pid function
6ff6b93
Fixesrust-lang#44971

tmccombs force-pushed the pid branch from fdbf940 to 6ff6b93Compare October 6, 2017 07:16

cuviper reviewed Oct 6, 2017
View reviewed changes

shepmaster added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 6, 2017

Add missing colon.
ba75758

sfackler added the T-libs-api [DEPRECATED; DO NOT USE] label Oct 7, 2017

rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Oct 7, 2017

Change current_pid to just id.
29b319b

shepmaster added S-waiting-on-team and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 15, 2017

rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Oct 25, 2017

kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-team labels Oct 25, 2017

kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 25, 2017

kennytm added a commit to kennytm/rust that referenced this pull request Oct 25, 2017
Rollup merge ofrust-lang#45059- tmccombs:pid, r=alexcrichton
bea6136
Add current_pid function Fixesrust-lang#44971

kennytm mentioned this pull request Oct 25, 2017
Rollup of 7 pull requests #45532
Merged

bors added a commit that referenced this pull request Oct 25, 2017
Auto merge of#45532- kennytm:rollup, r=kennytm
e847f30
Rollup of 7 pull requests - Successful merges: #45059, #45212, #45398, #45483, #45496, #45508, #45526 - Failed merges:

bors merged commit 29b319b into rust-lang:masterOct 26, 2017

tmccombs deleted the pid branch April 3, 2018 01:30

Uh oh!

Uh oh!

Conversation

tmccombs commented Oct 6, 2017

Uh oh!

rust-highfive commented Oct 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clarfontheyOct 7, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmccombsOct 7, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sfackler commented Oct 7, 2017

Uh oh!

sfackler commented Oct 7, 2017

Uh oh!

rfcbot commented Oct 7, 2017• edited by aturon Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexcrichton commented Oct 8, 2017

Uh oh!

carols10cents commented Oct 16, 2017

Uh oh!

kennytm commented Oct 25, 2017

Uh oh!

aturon commented Oct 25, 2017

Uh oh!

rfcbot commented Oct 25, 2017

Uh oh!

alexcrichton commented Oct 25, 2017

Uh oh!

bors commented Oct 25, 2017

Uh oh!

bors commented Oct 26, 2017

Uh oh!

bors commented Oct 26, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

14 participants

clarfontheyOct 7, 2017•
edited
Loading

tmccombsOct 7, 2017•
edited
Loading

rfcbot commented Oct 7, 2017•
edited by aturon
Loading