Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make OpaqueIpcReceiver useful #249

Merged
merged 2 commits into from
Dec 20, 2019
Merged

Make OpaqueIpcReceiver useful #249

merged 2 commits into from
Dec 20, 2019

Conversation

Manishearth
Copy link
Member

Currently it can't be converted to an IpcReceiver and isn't serializable, which makes it rather useless.

r? @pcwalton

@pcwalton
Copy link
Contributor

pcwalton commented Dec 18, 2019 •
edited

I'm somewhat uncomfortable reviewing this, because I don't recall the design constraints of OpaqueIpcReceiver anymore.

That said, I'm not opposed; I'm just worried about the design constraints here.

@Manishearth
Copy link
Member Author

cc @antrik

@antrik
Copy link
Contributor

antrik commented Dec 20, 2019

I'm not sure I can help much with this: I never worked much with that part of of the code base. There may or may not have been a time where I fully understood the idea behind OpaqueIpcSender and OpaqueIpcReceiver -- but if there was, that's been a while ago...

As far as I can tell, the proposed change simply brings OpaqueIpcReceiver in line with OpaqueIpcSender? While that seems reasonable on the surface, I really can't tell whether there was some rationale why it hasn't been like that from the beginning...

What's the use case, anyway?

One thing I can say though is that OpaqueIpcReceiver is being used in router.rs -- so it's certainly not "useless" as is...

jdm
jdm approved these changes Dec 20, 2019
View reviewed changes
Copy link
Member

@jdm jdm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the absence of anyone else, these changes look fine to me.

@jdm
Copy link
Member

jdm commented Dec 20, 2019

#250 will need to merge before this one can.

@jdm
Copy link
Member

jdm commented Dec 20, 2019

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit 794076b has been approved by jdm

@highfive highfive assigned jdm and unassigned pcwalton Dec 20, 2019
bors-servo pushed a commit that referenced this pull request Dec 20, 2019
Auto merge of #249 - Manishearth:opaque-receiver, r=jdm
5d0ec95 
Make OpaqueIpcReceiver useful

Currently it can't be converted to an IpcReceiver and isn't serializable, which makes it rather useless.

r? @pcwalton
@bors-servo
Copy link
Contributor

⌛ Testing commit 794076b with merge 5d0ec95...

@bors-servo bors-servo mentioned this pull request Dec 20, 2019
Merged
@bors-servo
Copy link
Contributor

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

@bors-servo bors-servo merged commit 794076b into servo:master Dec 20, 2019
@Manishearth Manishearth deleted the opaque-receiver branch December 20, 2019 22:44
@oli-cosmian
Copy link

Could someone publish a new version with this change included?

@jdm
Copy link
Member

jdm commented Jan 14, 2020

Opened #253 to start the process.

@Manishearth
Copy link
Member Author

This seems to already be included? https://docs.rs/ipc-channel/0.13.0/ipc_channel/ipc/struct.OpaqueIpcReceiver.html

@oli-cosmian
Copy link

I just was confused. This PR was merged after Cargo.toml was last changed. Sorry for the noise

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdm jdm jdm approved these changes

Assignees

@jdm jdm

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@Manishearth @pcwalton @antrik @jdm @bors-servo @oli-cosmian @highfive