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

Fixes issue #3891 #3923

Merged
merged 3 commits into from
Nov 8, 2012
Merged

Fixes issue #3891 #3923

merged 3 commits into from
Nov 8, 2012

Conversation

am0d
Copy link
Contributor

@am0d am0d commented Nov 6, 2012

There should be just one commit in here - if not, I'll try to fix that, but I can't really work out how to get rid of all the extra ones.

@am0d
Copy link
Contributor Author

am0d commented Nov 6, 2012

Argh! Can someone give me a tip on how to git rid of all the extra commits? I read the manuals for git, but it didn't make any sense to me.

@brson
Copy link
Contributor

brson commented Nov 6, 2012

Thanks!

git rebase mozilla/incoming should eliminate all those merges. When you push again you will have to do push -f because rebasing will change your history. In general I suggest resisting the urge to repeatedly merge from master while working on a development branch, because it results in this kind of noisy history. I typically do the work based off of a single upstream revision and submit the pull request without any merges.

Can you add a test for this? I realize the tests for this module are difficult, using a bunch of conditional compilation, but it's important. Note that every network test needs to use a different port number (or else they will fail randomly by failing to bind the port) and the ones in use aren't obvious.

I see whitespace at the end of lines in this patch which will cause 'make check' to fail.

@am0d
Copy link
Contributor Author

am0d commented Nov 6, 2012

Thanks @brson for the suggestions.
I can add a test for some of it, but not for the crash, since that could only be triggered by running with debug level logging on the module. I'll get a test written today and push that up with it.

This fixes rust-lang#3891.

Also removed debug!(...) statement from socket destructor which causes a
crash when the logging level is set to debug.
@am0d
Copy link
Contributor Author

am0d commented Nov 8, 2012

@brson I've fixed the whitespace issues now, and also added a test for the change. I couldn't write a test for the crash, which would only occur with debug level logging turned on - I hope that this is okay though.

@brson
Copy link
Contributor

brson commented Nov 8, 2012

@am0d that's great! thank you.

brson added a commit that referenced this pull request Nov 8, 2012
@brson brson merged commit 1702024 into rust-lang:incoming Nov 8, 2012
RalfJung pushed a commit to RalfJung/rust that referenced this pull request Oct 5, 2024
Refactor ``return_read_bytes_and_count`` and ``return_written_byte_count_or_error``

Fixes rust-lang#3904

This PR
- separate the error logic from ``return_read_bytes_and_count`` and ``return_written_byte_count_or_error`` into a helper function ``set_last_error_and_return``.
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.

2 participants