Add docs to ipc #153

dlrobertson · 2017-02-26T18:22:44Z

Add documentation to the structures in the ipc module.

dlrobertson · 2017-02-26T18:24:01Z

dlrobertson · 2017-02-26T18:34:56Z

Opened #154

antrik · 2017-02-27T08:50:28Z

Huh, highfive on strike?...

Anyway, before I get to take a closer look, just some generic remarks for now:

I don't think there is any value in having separate commits for each type. I'd say only the crate-level documentation should be separate.
Some of the descriptions essentially just seem to echo what the signatures already tell... The descriptions should try to explain the purpose of the various items, not the types they deal with etc.
I believe I have read that a doc comment should always start with a single proper sentence (ending with a period) briefly describing the item, as this is extracted as the short description... Don't know how strict the parser really is, though.

BTW, I'm not really familiar with rustdoc -- is there some easy way to quickly check how the result looks like?

dlrobertson · 2017-02-27T15:39:26Z

Good comments. I'll work on incorporating them later.

is there some easy way to quickly check how the result looks like?

cargo doc or if you're lazy cargo doc --open (essentially cargo doc; xdg-open ./path/to/doc/index.html)

dlrobertson · 2017-03-05T00:36:56Z

I don't think there is any value in having separate commits for each type. I'd say only the crate-level documentation should be separate.

Updated.

Some of the descriptions essentially just seem to echo what the signatures already tell... The descriptions should try to explain the purpose of the various items, not the types they deal with etc.

I tried to update this, but for most functions I didn't change much. E.g. for IpcReceiver::try_recv I left it as "Non-blocking receive". I think we need some sort of documentation here, but I'm not sure that much more would be hlepful.

I believe I have read that a doc comment should always start with a single proper sentence (ending with a period) briefly describing the item, as this is extracted as the short description... Don't know how strict the parser really is, though.

Yes, it doesn't appear to be too strict. I tried to adhere to this, but for functions like recv and try_recv I couldn't think of much more to write... I can think about it more and/or add more documentation if it is needed though.

bors-servo · 2017-06-15T15:43:16Z

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

dlrobertson · 2018-05-13T18:00:19Z

Updated and rebased on master. Is this still something that is worth merging?

antrik

Sorry, I was busy with something else when you originally sent this, and didn't remember to check back...

Yes, of course this is worth merging! The examples in particular are super valuable :-)

Should you feel unable or unwilling to address some of my comments, we can merge it nevertheless -- after all, I can always submit a PR with proposed improvements later on...

BTW, I think the commit message for the second commit doesn't really fit any more?...

antrik · 2018-05-14T03:02:54Z

src/ipc.rs

@@ -336,6 +571,9 @@ impl IpcSelectionResult {
    }
 }

+/// Structure used to represend a readable message from an [IpcSender].


I'd call it a raw received message before deserialisation, or something along these lines; and mention that it needs to be processed with the to() method to get a usable message.

(Also, typo: "represend" -> "represent"...)

antrik · 2018-05-14T03:02:54Z

src/ipc.rs

@@ -367,6 +605,7 @@ impl OpaqueIpcMessage {
        }
    }

+    /// Cast the data in the contained message to the inferred type.


I'm not sure it's a good idea to call this a cast... After all, rather than just changing types, it actually deserialises the raw received message into the requested type; thus recreating the original message that was passed into the channel. (Assuming the specified type actually matches the channel type... AIUI, there is not type checking here. Ouch. Definitely should mention that in the documentation...)

Note that I'd call the type "requested" or "specified" or something like that, rather than "inferred": while it's true that it will typically be inferred at the call site, that's not something the method itself cares about AIUI?

antrik · 2018-05-14T03:02:54Z

src/lib.rs

@@ -11,6 +11,33 @@
 			feature(mpsc_select))]
 #![cfg_attr(all(feature = "unstable", test), feature(specialization))]

+//! An implementation of the Rust channel API (a form of communicating sequential
+//! processes, CSP) over the native OS abstractions. Under the hood, this API uses


The important aspect IMHO is that ipc-channel implements the channel API across process boundaries. The fact that it abstracts over OS-specific IPC mechanisms is certainly worth mentioning, but more as a side note...

(I do realise that you took that introduction from README.md -- but note that while README.md elaborates on this only in the second paragraph, the create description actually reads "A multiprocess drop-in replacement for Rust channels"...)

Quite frankly, I'm also sceptical about the reference to CSP. While channels can be used as one of the building blocks for CSP, I wouldn't consider them "an implementation of CSP" -- unless I'm more confused about the exact meaning of this term than I thought...

antrik · 2018-05-14T03:02:54Z

src/lib.rs

+//! Mach ports on Mac and file descriptor passing over Unix sockets on Linux. The
+//! serde library is used to serialize values for transport over the wire.
+//!
+//! For more detail, see [IpcReceiver], [IpcSender], and [IpcReceiverSet].


Not sure this remark is very helpful...

(Also, I'd say the main entry point is actually channel()?)

antrik · 2018-05-14T03:02:54Z

src/lib.rs

+//! # Features
+//! ## `force-inprocess`
+//!
+//! Force the `inprocess` backend to be used instead of the OS specific backend.


Since this isn't documented anywhere else as far as I'm aware, I'd say we should explicitly mention here that this is a dummy back-end, that behaves much like the real ones, but doesn't actually work between different processes...

antrik · 2018-05-14T03:02:54Z

src/ipc.rs

+/// #
+/// # let (tx, rx) = ipc::channel().unwrap();
+/// # let (embedded_tx, embedded_rx) = ipc::channel().unwrap();
+/// # let data = vec![0x45, 0x6d, 0x62, 0x65, 0x64, 0x64, 0x65, 0x64, 0x00];


I think you could use a simple array here?...

(Applies to other examples too.)

antrik · 2018-05-14T03:02:54Z

src/ipc.rs

+/// # use ipc_channel::ipc;
+/// #
+/// # let (tx, rx) = ipc::channel().unwrap();
+/// # let (embedded_tx, embedded_rx) = ipc::channel().unwrap();


I wonder whether it wouldn't be clearer to make these visible?

antrik · 2018-05-14T03:02:54Z

src/ipc.rs

@@ -158,13 +310,21 @@ impl<T> Clone for IpcSender<T> where T: Serialize {
 }

 impl<T> IpcSender<T> where T: Serialize {
+    /// Create an [IpcSender] connected to a previously defined [IpcOneShotServer].


I think it would be clearer to mention that the one-shot server was created elsewhere (typically in a different process), rather than just earlier -- after all, that's the whole point of this mechanism...

Probably should also explain that the connection is established using an identifier string that was also used in the creation of the server, and passed between processes by other means (such as command line arguments or environment variables), in order to bootstrap an initial IPC connection between different processes.

(Arguably part of that rather belongs in the documentation of IpcOneShotServer -- but since you aren't adding any documentation for that one... :-) )

antrik · 2018-05-14T03:02:54Z

src/ipc.rs

    pub fn connect(name: String) -> Result<IpcSender<T>,Error> {
        Ok(IpcSender {
            os_sender: try!(OsIpcSender::connect(name)),
            phantom: PhantomData,
        })
    }

+    /// Send data to the connected [IpcReceiver] or [IpcOneShotServer].


I suspect bringing up IpcOneShotServer here will only cause confusion. The message is not actually sent to the server: it is sent to a normal receiver -- which just happens to be held by the server, until it is handed out to the actual user as a result of the accept() call. From the sender's point of view, only the connect() call deals with the server at all.

antrik · 2018-05-14T03:02:54Z

src/ipc.rs

@@ -191,6 +351,7 @@ impl<T> IpcSender<T> where T: Serialize {
        })
    }

+    /// Return a wrapper for the OS specific backend.


Same remark as for IpcReceiver.to_opaque().

(Though frankly I have no idea what OpaqueIpcSender is actually supposed to be useful for... Looks entirely unused to me. Probably should just drop it...)

bors-servo · 2018-08-09T17:48:24Z

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


        Improve docs of prominent functions and structures

- Add crate level documentation that discusses the overall goals of the crate and the features it implements. - Add basic documentation to the ipc::channel and ipc::bytes_channel functions. - Add basic documentation to the main structures and their commonly used methods.

dlrobertson · 2018-12-03T00:06:56Z

Updated to address comments... almost two years later 😄

jdm · 2018-12-03T07:23:35Z

@bors-servo r+
Ship it!

bors-servo · 2018-12-03T07:23:36Z

📌 Commit 202b7ab has been approved by jdm

bors-servo · 2018-12-03T07:23:39Z

⌛ Testing commit 202b7ab with merge fe91787...


        Auto merge of #153 - dlrobertson:add_docs_to_ipc, r=jdm

Add docs to ipc Add documentation to the structures in the `ipc` module.

bors-servo · 2018-12-03T07:28:34Z

☀️ Test successful - status-appveyor, status-travis
Approved by: jdm
Pushing fe91787 to master...

dlrobertson force-pushed the dlrobertson:add_docs_to_ipc branch from 0007a1e to 9e98dc9 Feb 28, 2017

highfive added the S-awaiting-review label Feb 28, 2017

dlrobertson force-pushed the dlrobertson:add_docs_to_ipc branch from 9e98dc9 to 9e70e8d Mar 5, 2017

dlrobertson force-pushed the dlrobertson:add_docs_to_ipc branch from 9e70e8d to 1414c1a Mar 5, 2017

dlrobertson force-pushed the dlrobertson:add_docs_to_ipc branch 2 times, most recently from ac30d15 to e3825b1 May 13, 2018

antrik reviewed May 14, 2018

View changes

dlrobertson force-pushed the dlrobertson:add_docs_to_ipc branch from e3825b1 to 2d520bb May 14, 2018

highfive added the S-needs-rebase label Aug 9, 2018

dlrobertson force-pushed the dlrobertson:add_docs_to_ipc branch from 2d520bb to 8fd1cbd Dec 2, 2018

dlrobertson force-pushed the dlrobertson:add_docs_to_ipc branch from 8fd1cbd to 202b7ab Dec 3, 2018

highfive assigned jdm Dec 3, 2018

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Dec 3, 2018

bors-servo merged commit 202b7ab into servo:master Dec 3, 2018
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Dec 3, 2018

dlrobertson deleted the dlrobertson:add_docs_to_ipc branch Dec 3, 2018

servo / ipc-channel

Add docs to ipc #153

Add docs to ipc #153

dlrobertson commented Feb 26, 2017

dlrobertson commented Feb 26, 2017

dlrobertson commented Feb 26, 2017

antrik commented Feb 27, 2017

dlrobertson commented Feb 27, 2017

dlrobertson commented Mar 5, 2017

bors-servo commented Jun 15, 2017

dlrobertson commented May 13, 2018

antrik left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented Aug 9, 2018

dlrobertson commented Dec 3, 2018

jdm commented Dec 3, 2018

bors-servo commented Dec 3, 2018

bors-servo commented Dec 3, 2018

bors-servo commented Dec 3, 2018

servo / ipc-channel

Join GitHub today

Add docs to ipc #153

Add docs to ipc #153

Conversation

dlrobertson commented Feb 26, 2017

dlrobertson commented Feb 26, 2017

dlrobertson commented Feb 26, 2017

antrik commented Feb 27, 2017

dlrobertson commented Feb 27, 2017

dlrobertson commented Mar 5, 2017

bors-servo commented Jun 15, 2017

dlrobertson commented May 13, 2018

antrik left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented Aug 9, 2018

dlrobertson commented Dec 3, 2018

jdm commented Dec 3, 2018

bors-servo commented Dec 3, 2018

bors-servo commented Dec 3, 2018

bors-servo commented Dec 3, 2018