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.

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

mqueue: change types to allow more cases #271

Merged
merged 1 commit into from
Feb 20, 2016

Conversation

abbradar
Copy link
Contributor

Part of #270, fixed according to @kamalmarhubi's comments.

@kamalmarhubi
Copy link
Member

@homu r+

@homu
Copy link
Contributor

homu commented Feb 19, 2016

📌 Commit 62ec478 has been approved by kamalmarhubi

@homu
Copy link
Contributor

homu commented Feb 19, 2016

⌛ Testing commit 62ec478 with merge f2343cd...

homu added a commit that referenced this pull request Feb 19, 2016
mqueue: change types to allow more cases

Part of #270, fixed according to @kamalmarhubi's comments.
@kamalmarhubi
Copy link
Member

Looks like I led you astray suggesting as *const _, and you'll need the full type. :/

@abbradar
Copy link
Contributor Author

Hm, why had it compiled for me? I use Rust unstable (coming from Haskell also, we have partial type signatures there so I wasn't on guard).

@kamalmarhubi
Copy link
Member

@abbradar are you on OS X? Looks like it even failed on nightly on Linux: https://travis-ci.org/nix-rust/nix/jobs/110474199

@kamalmarhubi
Copy link
Member

Oh the nightly failures are different. Interesting.

@abbradar
Copy link
Contributor Author

No, on Linux -- just tried setting i32 instead of _ and it (EDIT) hasn't worked for me. I use very fresh nightly -- from several days ago, so maybe they have improved type inference.

@kamalmarhubi
Copy link
Member

@homu retry

@homu
Copy link
Contributor

homu commented Feb 19, 2016

⌛ Testing commit 62ec478 with merge 48aa78b...

homu added a commit that referenced this pull request Feb 19, 2016
mqueue: change types to allow more cases

Part of #270, fixed according to @kamalmarhubi's comments.
@kamalmarhubi
Copy link
Member

@homu r+

@homu
Copy link
Contributor

homu commented Feb 19, 2016

💡 This pull request was already approved, no need to approve it again.

@homu
Copy link
Contributor

homu commented Feb 19, 2016

📌 Commit 62ec478 has been approved by kamalmarhubi

@homu
Copy link
Contributor

homu commented Feb 19, 2016

💡 This pull request was already approved, no need to approve it again.

@kamalmarhubi
Copy link
Member

@homu r-

@kamalmarhubi
Copy link
Member

@homu r+

@homu
Copy link
Contributor

homu commented Feb 19, 2016

📌 Commit 62ec478 has been approved by kamalmarhubi

@kamalmarhubi
Copy link
Member

(Sorry for the noise, new to @homu around here!)

@abbradar
Copy link
Contributor Author

I have a fix with a fully specified type ready, so if she fails I'll force-push it and have you know.

@homu
Copy link
Contributor

homu commented Feb 19, 2016

💡 This pull request was already approved, no need to approve it again.

1 similar comment
@homu
Copy link
Contributor

homu commented Feb 19, 2016

💡 This pull request was already approved, no need to approve it again.

@kamalmarhubi
Copy link
Member

@homu retry

@homu
Copy link
Contributor

homu commented Feb 19, 2016

⌛ Testing commit 62ec478 with merge 68fe26b...

homu added a commit that referenced this pull request Feb 19, 2016
mqueue: change types to allow more cases

Part of #270, fixed according to @kamalmarhubi's comments.
@abbradar
Copy link
Contributor Author

I've force-pushed the version with full type specification. Let's try it!

@kamalmarhubi
Copy link
Member

@homu retry

@kamalmarhubi
Copy link
Member

I think I've confused @homu on this one.

@kamalmarhubi
Copy link
Member

Oh right, it unapproves if the PR changes.

@homu r+ 05f933b

@kamalmarhubi
Copy link
Member

Looks like you need to fix uses in test/test_mq.rs?

@abbradar
Copy link
Contributor Author

Yeah, I've learnt to use cargo test only now ^_^". Sorry, fixed that.

@kamalmarhubi
Copy link
Member

@homu retry

@kamalmarhubi
Copy link
Member

@homu r+ 8ca6d82

@abbradar
Copy link
Contributor Author

@homu keeps silent for some reason -- should she be like this?

homu added a commit that referenced this pull request Feb 20, 2016
mqueue: change types to allow more cases

Part of #270, fixed according to @kamalmarhubi's comments.
@homu
Copy link
Contributor

homu commented Feb 20, 2016

⌛ Testing commit 8ca6d82 with merge 6cad03a...

@homu
Copy link
Contributor

homu commented Feb 20, 2016

☀️ Test successful - status

@homu homu merged commit 8ca6d82 into nix-rust:master Feb 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants