Improve Wait Documentation #651

mmstick · 2017-07-07T03:20:02Z

As the title states, improves the documentation for the wait module by providing documentation where there is currently no documentation. Also, changed the signature of waitpid to do as the PID parameter currently does (Into<Option<T>>), so that you don't have to always type Some every time you want to supply a flag -- but you can still provide a None value.

Closes #643.

Susurrus · 2017-07-07T03:30:14Z

src/sys/wait.rs

-        Some(bits) => bits.bits(),
-        None => 0
-    };
+    let option_bits = options.into().map_or(0, |bits| bits.bits());


bits.bits() reads weird, can you just shorten it to b.bits()?

Susurrus · 2017-07-07T03:30:47Z

src/sys/wait.rs


-    let res = unsafe { ffi::waitpid(pid.into().unwrap_or(Pid::from_raw(-1)).into(), &mut status as *mut c_int, option_bits) };
+    let res = unsafe {
+        ffi::waitpid(pid.into().unwrap_or (


No space before the last (

Susurrus · 2017-07-07T03:31:37Z

src/sys/wait.rs

        0 => StillAlive,
        res => decode(Pid::from_raw(res), status),
    })
 }

+/// Equivalent to `waitpid(-1, None)`, which waits on any child process of the current process.


This is confusing because it's actually equivalent to waitpid(None, None).

The None actually converts to -1 inside the function, so it's better to state exactly what None means, especially given that the original C function doesn't support algebraic data types like None.

It's still confusing because the code doesn't match the comment until you read it a little more thoroughly. I'm a fan of the "principle of least surprise": I was surprised to read that comment and then see that the code didn't match that, though I later realized they're equivalent. I think it'd be better here to just remove the waitpid... part and just have a description of what it does. A more descriptive comment below could explain that it's equivalent, but it's definitely not necessary on the title line.

Susurrus · 2017-07-07T03:32:24Z

src/sys/wait.rs

    Stopped(Pid, Signal),
    #[cfg(any(target_os = "linux", target_os = "android"))]
    PtraceEvent(Pid, Signal, c_int),
+    /// Signifies that the process received a `SIGCONT` signal, and thus continued.
    Continued(Pid),
    StillAlive


I'd like to get some doc comments on StillAlive and PtraceEvent since we're here. Are you stuck on understanding those or did you just forget to add them?

I couldn't find any mention of StillAlive or PtraceEvents mentioned inthe documentation for waitpid, so I'm not sure what they do.

Susurrus · 2017-07-07T03:33:31Z

src/sys/wait.rs

@@ -42,11 +55,15 @@ const WSTOPPED: WaitPidFlag = WUNTRACED;

 #[derive(Eq, PartialEq, Clone, Copy, Debug)]


It'd be good to have a comment for the entire enum.

Susurrus · 2017-07-07T03:34:59Z

src/sys/wait.rs

@@ -25,13 +27,24 @@ libc_bitflags!(
          target_os = "android"))]


Rather than duplicating this libc_bitflags, could you condense these into one? Before each item you can put the proper #[cfg so that it still compiles properly. This way we aren't duplicating declarations and comments.

Susurrus · 2017-07-07T03:35:21Z

src/sys/wait.rs

@@ -25,13 +27,24 @@ libc_bitflags!(
          target_os = "android"))]
 libc_bitflags!(


I'd like to see a doccomment for WaitPidFlag here since you've already documented (nearly) every item.

Susurrus · 2017-07-07T03:44:03Z

src/sys/wait.rs

+/// - If the value of the PID is `0`, it will wait on any child process that has the same
+/// group ID as the current process.
+/// - If the value of the PID is `-1`, it will wait on any child process of the current process.
+/// - If the value of the PID is `None`, the value of PID is set to `-1`.


Combine this line with the above. But then, what's the point of making Pid and Option type rather than leaving it numeric if there are two None cases? In reality, this should take an enum of these different groups, which are then converted to a numeric pid_t internally. Like:

pub enum PidGroup { ChildPid(Pid), ProcessGroupChild(Pid), GroupChild, CurrentProcessChild, }

Then the user uses one of those to specify the option. What do you think about that?

I think that'd be a much better option, although it would require a version bump due to breaking the existing API. I think the best would be to have the parameter value as Into<PidGroup>, where PidGroup could also implement From<i32>, so that we could continue using i32 values as in C in addition.

Or I suppose have the parameter as Into<i32> where PidGroup implements Into<i32>.

We already have breaking changes in this release, and I'm not afraid to have breaking changes when it results in improved APIs. The 0.9 release that will come out soon will have breaking changes, and I don't see that as a problem as long as they're well documented.

Susurrus · 2017-07-07T03:44:31Z

Awesome, great work starting this! We always could use more docs, so thanks for the PR!

I've got a few general comments beyond what I left about your code:

Could you add a module-level comment at the top? These start with //! and have a one-line summary at the beginning and then a space and a more detailed summary below as necessary.
Could you remove the ffi module and instead use libc::waitpid()
Could you add to the CHANGELOG under the Changed section that the function arguments to waitpid have changed (see my comment about the function arguments before doing this).

Susurrus · 2017-07-07T03:45:13Z

@asomers Another FreeBSD build that failed, and the merge branch here is also master, so I think that's definitely the problem.

asomers · 2017-07-07T04:10:25Z

@Susurrus I opened an issue for the master branch thing: #652 . I think it should be fixed now; we'll see as soon as mmstick pushes again.

mmstick · 2017-07-07T04:14:38Z

I'll make a push within 30 mins with all the changes. What do you think though? Should the pid parameter of waitpid be Into<pid_t>, where either a pid_t or PidGroup value could be supplied; or Into<PidGroup>, that converts a given pid_t into a PidGroup, or takes a PidGroup, and then the function turns the PidGroup back into a pid_t internally?

asomers · 2017-07-07T04:16:17Z

src/sys/wait.rs

    Exited(Pid, i8),
+    /// Signifies that the process was killed by a signal, providing the PID and associated signal.


Could you also please document the bool member?

Once I figure out what the bool parameter signifies.

Susurrus · 2017-07-07T04:29:06Z

No rush on the changes, whenever you've got time is fine. I'm just at a computer, so you're getting nice quick responses! :-)

As to your question, it might take some test code that plays with the API to see what's most ergonomic, but I'm inclined to think taking a PidGroup and converting it internally makes the most sense. There's no need for Into if we use that enum. Since PidGroup group can wrap Pid types, there's no need for Into<PidGroup>.

Susurrus · 2017-07-07T15:31:29Z

src/sys/wait.rs

    Continued(Pid),
+    /// if `WNOHANG` was set, this value is returned when no children have changed state.


Please capitalize if here.

Susurrus · 2017-07-07T15:32:30Z

src/sys/wait.rs

+/// Designates whether the supplied `Pid` value is a process ID, process group ID,
+/// specifies any child of the current process's group ID, or any child of the current process.
+pub enum PidGroup {
+    ProcessID(u32),


I imagine this would be a lot cleaner if you used the Pid type here directly instead of u32. Any reason you didn't?

It's kind of odd to use something other than u32, as the purpose of separating the variants is to eliminate the need to express meaning through negative values, as Pid is basically i32. A negative value indicates a process group ID, whereas a positive value indicates a process ID. If the process ID and group ID are going to get their own variants, it wouldn't make sense to require them to retain their positive and negative values. If you retained the Pid value, then there'd be less meaning to having both a PID and PGID variant when they'd always get the same value and that value would be treated the same by functions.

If it retained Pid/i32 then it wouldn't make a difference if you wrote:

match waitpid(ProcessID(pid), WUNTRACED) { }

Or if you wrote

match waitpid(ProcessGroupID(pid), WUNTRACED) {}

The behavior would be entirely dependent upon whether the value supplied to either variant is positive or negative.

Especially when I would, for example, create a PidGroup manually in Ion by just supplying my existing u32(Linux/BSD)/usize(Redox) values that I have stored in the shell to the ProcessGroupID variant, rather than having to do as I do now and convert the u32 into an i32, and then set the negative bit to signify to the function that I'm wanting to wait/signal a process group. Being able to just designate the PGID with a positive value is more idiomatic.

match waitpid(ProcessGroupID(pid), WUNTRACED) { ... }

I'm not certain we're quite on the same page with understanding this issue based on your comment here: I'm trying to develop a solution where you never need to negate the pid as well. My understanding that when using waitpid() that you will already have a process ID, the question is what does it mean. That extra meaning is why there's the enum type, so even though ProcessID(pid) and ProcessGroupId(pid) have the same process id within them, they mean different things. And internally the values would be mapped as the libc::waitpid function expected. So the point is that the user shouldn't be doing the negation just as you suggested, the value used for ProcessId and ProcessGroupId is the same raw pid value.

What we do need to make sure is that the raw value is "correct". For example, the pid should be positive and non-zero for both of those types, so that should be checked. We can either embed this into the enum type (which I don't know a good implementation for) or embed it within waitpid(), which is the path I'd suggest.

Is that more clear? Because I think we agree we're just losing something in translation.

It seems, then, that it'd be better if Pid was mapped to an unsigned integer rather than a signed integer that can be constructed from a negative value, if we need to guarantee that a negative value cannot be mapped. Otherwise, there'd be a cost in having to always check if the value of ProcessGroupID is negative, and then setting the negative bit if it's positive, else use the value as is. If it was an unsigned integer, then you'd always know that the PGID variant just needs to be converted into a signed integer and have it's negative bit set before passing it to the function. Process IDs are never negative values to begin with.

pid_t is the underlying type and that's defined by the platform, we don't change that. And yes, there will be a runtime cost of checking these values, but we'd need to check them even if they're unsigned since 0 doesn't make sense for these enum types either.

Susurrus · 2017-07-07T15:32:50Z

src/sys/wait.rs

-pub fn waitpid<P: Into<Option<Pid>>>(pid: P, options: Option<WaitPidFlag>) -> Result<WaitStatus> {
-    use self::WaitStatus::*;
+#[derive(Debug, Copy, Clone, Eq, PartialEq, Hash)]
+/// Designates whether the supplied `Pid` value is a process ID, process group ID,


Please put the comments above #cfg attributes.

Susurrus · 2017-07-07T15:33:16Z

src/sys/wait.rs

        WUNTRACED,
+        #[cfg(any(target_os = "linux",


Please put cfg statements on one line if they're under 100 characters.

Susurrus · 2017-07-07T15:33:49Z

src/sys/wait.rs

+    AnyChild,
+}
+
+impl From<Pid> for PidGroup {


The Froms for PidGroup should be removed in favor on embedded Pids directly into the specific enum types.

Susurrus · 2017-07-07T15:36:12Z

Looking pretty good, the tests didn't get updated. Please fix those on your next update.

mmstick · 2017-07-07T17:44:01Z

I believe that I have all concerns addressed now.

Susurrus · 2017-07-07T18:02:09Z

src/sys/wait.rs

@@ -267,13 +253,14 @@ impl From<i32> for PidGroup {
    }
 }

-impl Into<i32> for PidGroup {
-    fn into(self) -> i32 {
+impl Into<pid_t> for PidGroup {


This is a public implementation that I don't think we want to support. While the first argument of waitpid is a pid_t type, it's actually not a "proper" one. As in it's different than the pid_t you'd get or pass to many other functions, so it's in essence a different type (it's just overloaded in C because of language limitations). I think instead we want to just hardcode this inside of waitpid or implement it as a private function (so it can be unit tested) instead.

Susurrus · 2017-07-07T18:02:33Z

src/sys/wait.rs

-            PidGroup::ProcessGroupID(pid) => -(pid as i32),
-            PidGroup::AnyGroupChild       => 0,
-            PidGroup::AnyChild            => -1
+            PidGroup::ProcessID(pid) if pid < Pid::from_raw(-1)      => -pid_t::from(pid),


Please don't align the => in match arms, just have them surrounded by one space on each size.

Susurrus · 2017-07-07T18:03:14Z

src/sys/wait.rs

@@ -233,32 +233,18 @@ fn decode(pid : Pid, status: i32) -> WaitStatus {
 /// specifies any child of the current process's group ID, or any child of the current process.
 #[derive(Debug, Copy, Clone, Eq, PartialEq, Hash)]


Can you alphabetize the derived traits?

Susurrus · 2017-07-07T21:06:44Z

src/sys/wait.rs

+///
+/// # Usage Notes
+///
+/// - If the value of PID is greater than `0`, it will wait on the child with that PID.


Can you update these docs now that the pid argument type has changed?

Susurrus · 2017-07-07T21:06:58Z

src/sys/wait.rs

-pub fn waitpid<P: Into<Option<Pid>>>(pid: P, options: Option<WaitPidFlag>) -> Result<WaitStatus> {
+/// Designates whether the supplied `Pid` value is a process ID, process group ID,
+/// specifies any child of the current process's group ID, or any child of the current process.
+#[derive(Debug, Copy, Clone, Eq, PartialEq, Hash)]


Can you alphabetize these?

Susurrus · 2017-07-07T21:07:23Z

src/sys/wait.rs

    StillAlive
 }

-#[cfg(any(target_os = "linux",
-          target_os = "android"))]
+#[cfg(any(target_os = "linux", target_os = "android"))]


Please alphabetize the list of oses here and everywhere else cfg is used within the scope of this PR.

Susurrus · 2017-07-07T21:07:50Z

src/sys/wait.rs

@@ -1,58 +1,72 @@
-use libc::{self, c_int};
+//! This module contains the `wait()` and `waitpid()` functions, which are used


I think something got left out here, it's an incomplete sentence.

Susurrus · 2017-07-10T23:16:36Z

src/sys/wait.rs

    }
 );

-#[cfg(any(target_os = "linux",
-          target_os = "android"))]
+#[cfg(any(target_os = "android", target_os = "linux"))]
 const WSTOPPED: WaitPidFlag = WUNTRACED;


This should actually be included in the WaitPidFlag bitflag declaration, yes?

Can it be? The other flags are automatically generated via the libc_bitflags! macro.

We don't want to be defining WSTOPPED in nix, instead it should be defined in the libc crate and we include it on appropriate platforms. It isn't defined in libc for android or linux however, so we'll need a PR for that.

Susurrus · 2017-07-11T02:02:00Z

@mmstick For fixing documentation errors, please test these locally first. Tests take 2 hours to run for us and you're pushing so many commits that we have a huge backlog in CI. I've already canceled a bunch of your old pushes because of it.

mmstick · 2017-07-11T04:28:08Z

That last commit fixed everything.

valpackett · 2017-07-11T14:00:56Z

src/sys/wait.rs

        WEXITED,
+        /// Report the status of any continued child process specified by pid whose status has not
+        /// been reported since it continued from a job control stop.
+        #[cfg(any(target_os = "android", target_os = "linux"))]
        WCONTINUED,


WCONTINUED is available pretty much everywhere. Definitely on freebsd, openbsd, bitrig, dragonfly, netbsd and solaris.

(Does nix even support solaris? it's waitid on illumos instead of waitpid…)

WCONTINUED is not mentioned in the macOS manpage, someone with macOS should check there…

It's defined for Mac within libc, so it should be fine on both macos and ios

Nice. Probably should just drop the cfg here

valpackett · 2017-07-11T14:02:44Z

src/sys/wait.rs

+        /// Leave the child in a waitable state; a later wait call can be used to again retrieve
+        /// the child status information.
+        #[cfg(any(target_os = "android", target_os = "linux"))]
+        WNOWAIT,


WNOWAIT is available on freebsd and netbsd.

valpackett · 2017-07-11T14:03:40Z

src/sys/wait.rs

        WUNTRACED,
+        /// Waits for children that have terminated.
+        #[cfg(any(target_os = "android", target_os = "linux"))]
        WEXITED,


WEXITED is available on freebsd and netbsd.

Susurrus · 2017-07-11T16:08:43Z

This also touches on changes made in #566. I think I'm going to try to get that PR merged first and then this can be rebased afterwards.

Susurrus

Would be great to get this in the 0.9 release given how much it improves documentation. It's pretty close, mostly just minor documentation changes at this point.

Susurrus · 2017-07-19T14:34:55Z

src/sys/wait.rs

+/// - If the value of the PID is `PidGroup::AnyChild`, it will wait on any child process of the
+///   current process.
+///
+/// # Possible Error Values


This should just be # Errors (see the Rust special documentation section)

Susurrus · 2017-07-19T14:35:53Z

src/sys/wait.rs

+    }
+}
+
+/// Waits for and returns events that are received from the given supplied process or process group


This should be a short descriptive line, then a blank line, then more description. If you run rust doc --no-deps --open and look at the generated output, you'll see why. The first line is used on the summary page, and the rest is shown on the detailed page.

Additionally this should link to the man page (see how other functions are documented).

Susurrus · 2017-07-19T14:36:11Z

src/sys/wait.rs

@@ -215,24 +262,84 @@ fn decode(pid : Pid, status: i32) -> WaitStatus {
    }
 }

-pub fn waitpid<P: Into<Option<Pid>>>(pid: P, options: Option<WaitPidFlag>) -> Result<WaitStatus> {
+/// Designates whether the supplied `Pid` value is a process ID, process group ID,


Same here with the documentation.

Susurrus · 2017-07-19T14:37:47Z

src/sys/wait.rs

+//! child management than the primitives provided by Rust's standard library, and are critical
+//! in the creation of shells and managing jobs on *nix platforms.
+//!
+//! # Example


This should be # Examples in line with the special sections syntax

Susurrus · 2017-07-19T14:38:20Z

src/sys/wait.rs

@@ -1,58 +1,105 @@
-use libc::{self, c_int};
+//! This module contains the `wait()` and `waitpid()` functions, which are used to wait on and


Short descriptive first line, then a blank line, then more description. Generate the docs locally with rust doc --no-deps --open to see what I'm talking about.

Susurrus · 2017-07-19T14:38:42Z

src/sys/wait.rs

+///
+/// # Possible Error Values
+///
+/// If this function returns an error, the error value will be one of the following:


You can just remove this sentence as it's superfluous.

Susurrus · 2017-07-19T14:39:46Z

src/sys/wait.rs

        0 => StillAlive,
        res => decode(Pid::from_raw(res), status),
    })
 }

+/// Waits on any child of the current process, returning on events that change the status of


Same thing here about a short descriptive first sentence. Also, it should provide a link to the man page (see how other functions are documented). Also it looks like the possible errors are a subset of waitpid's, so it'd be good to explicitly document them here.

Susurrus · 2017-07-19T14:43:33Z

Looks like this also needs a rebase.

I'd love to move forward with this and let #566 catch up later as it's currently stalled on the OP.

mmstick · 2017-07-20T00:49:36Z

Just performed a git pull --rebase upstream master.

mmstick · 2017-07-20T01:05:20Z

Performed requested changes to the documentation.

Susurrus

This also needs a rebase, to be squashed into a smaller number of commits (commits that just fix something that wasn't right in an earlier commit should be merged into the earlier commmits), and then a CHANGELOG entry for the change in the API of waitpid().

Susurrus · 2017-08-01T22:27:43Z

src/sys/wait.rs

+//!
+//! Manual Page: http://pubs.opengroup.org/onlinepubs/007908799/xsh/wait.html
+//!
+//! # Examples


Please provide a brief description of what this code does, either in text before the example or using comments within the code.

Susurrus · 2017-08-01T22:28:10Z

src/sys/wait.rs

+//!
+//! # Examples
+//!
+//! ```rust


Add ,no_run at the end since this code example will fail when run.

Susurrus · 2017-08-01T22:28:28Z

src/sys/wait.rs

+//!         Ok(_) => (),
+//!         Err(why) => {
+//!             println!("waitpid returned an error code: {}", why);
+//!             break


Add a semi-colon after break.

Susurrus · 2017-08-01T22:36:38Z

src/sys/wait.rs

+    AnyChild,
+}
+
+impl From<i32> for PidGroup {


This should be removed as it's unused and we shouldn't support this for our users.

Susurrus · 2017-11-16T03:59:20Z

@mmstick Any interest in cleaning this up a bit? We're close to merging and we would love to have this documentation in nix!

Susurrus · 2017-12-09T19:10:01Z

@mmstick Just pinging you again on this. Do you have time to wrap this up in the near future?

Noteworthy secondary changes included as a result of the primary documentation: * Additions to unistd::Pid to support waitpid calling conventions * Change the waitpid function signature (backward compatible) * Application of rustfmt to unistd.rs and sys::wait.rs Continued from work by * Marcin Mielniczuk <marmistrz.dev@gmail.com> * Michael Aaron Murphy <mmstickman@gmail.com> Closes nix-rust#654 and nix-rust#651

Susurrus suggested changes Jul 7, 2017

View reviewed changes

asomers mentioned this pull request Jul 7, 2017

buildbot screws up pull requests from a "master" branch #652

Closed

Susurrus suggested changes Jul 7, 2017

View reviewed changes

asomers reviewed Jul 7, 2017

View reviewed changes

mmstick force-pushed the master branch 3 times, most recently from 592bd52 to 5a3522f Compare July 7, 2017 07:55

Susurrus mentioned this pull request Jul 7, 2017

Document the PID argument in waitpid #654

Open

Susurrus suggested changes Jul 7, 2017

View reviewed changes

Susurrus suggested changes Jul 10, 2017

View reviewed changes

Susurrus mentioned this pull request Jul 10, 2017

Add waitpid flags for *BSD operating systems #668

Closed

valpackett reviewed Jul 11, 2017

View reviewed changes

Susurrus suggested changes Jul 19, 2017

View reviewed changes

mmstick added 2 commits July 19, 2017 20:43

Improve Wait Documentation

0a3b4dd

Implement PidGroup & More Documentation

79c19d8

mmstick added 12 commits July 19, 2017 20:43

PtraveEvent Documentation

ad4e138

Fix Tests & Comments

77e5d49

Require Runtime Checks on Pid Values in PidGroup

8607d93

Remove Into<pid_t> for PidGroup

384fb04

More Documentation Updates

ed82b31

Fix Document Build Errors

fb69fbb

Remove WSTOPPED

519303c

Fix Document Build Errors

ccfa6ee

More Doc Build Fixes

224d36d

Another Doc Build Fix

0711f21

Fix Document Build Errors

1b4715b

Add BSD targets

64dde10

mmstick force-pushed the master branch from 43f5f1a to 64dde10 Compare July 20, 2017 00:48

Improve Documentation

8632b56

Merge Upstream Changes

40db19b

mmstick force-pushed the master branch from 82bc59a to 40db19b Compare July 22, 2017 04:03

Susurrus suggested changes Aug 1, 2017

View reviewed changes

Susurrus closed this Dec 13, 2017

vkkoskie mentioned this pull request Jun 5, 2022

Document the sys::wait module #1732

Open

		@@ -42,11 +55,15 @@ const WSTOPPED: WaitPidFlag = WUNTRACED;

		#[derive(Eq, PartialEq, Clone, Copy, Debug)]

		@@ -25,13 +27,24 @@ libc_bitflags!(
		target_os = "android"))]
		libc_bitflags!(

		Exited(Pid, i8),
		/// Signifies that the process was killed by a signal, providing the PID and associated signal.

		Continued(Pid),
		/// if `WNOHANG` was set, this value is returned when no children have changed state.

		@@ -233,32 +233,18 @@ fn decode(pid : Pid, status: i32) -> WaitStatus {
		/// specifies any child of the current process's group ID, or any child of the current process.
		#[derive(Debug, Copy, Clone, Eq, PartialEq, Hash)]

		@@ -1,58 +1,72 @@
		use libc::{self, c_int};
		//! This module contains the `wait()` and `waitpid()` functions, which are used

		@@ -1,58 +1,105 @@
		use libc::{self, c_int};
		//! This module contains the `wait()` and `waitpid()` functions, which are used to wait on and

Improve Wait Documentation #651

Improve Wait Documentation #651

Conversation

mmstick commented Jul 7, 2017 • edited by Susurrus 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmstick Jul 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Susurrus commented Jul 7, 2017

Susurrus commented Jul 7, 2017

asomers commented Jul 7, 2017

mmstick commented Jul 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Susurrus commented Jul 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmstick Jul 7, 2017 • 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

Susurrus commented Jul 7, 2017

mmstick commented Jul 7, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Susurrus commented Jul 11, 2017

mmstick commented Jul 11, 2017

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

Susurrus commented Jul 11, 2017

Susurrus left a 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

Choose a reason for hiding this comment

Susurrus commented Jul 19, 2017

mmstick commented Jul 20, 2017

mmstick commented Jul 20, 2017

Susurrus left a 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

Susurrus commented Nov 16, 2017

Susurrus commented Dec 9, 2017

mmstick commented Jul 7, 2017 •

edited by Susurrus

Loading

mmstick Jul 7, 2017 •

edited

Loading

mmstick Jul 7, 2017 •

edited

Loading