Skip to content

Change traits to take &mut self, where appropriate #20

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

Merged
merged 9 commits into from
Oct 17, 2017
Merged

Change traits to take &mut self, where appropriate #20

merged 9 commits into from
Oct 17, 2017

Conversation

hannobraun
Copy link
Member

This pull request supersedes #16, which seems stalled. It is based on #19. Only the last two commits are actually relevant to this pull request itself. I suggest merging #19 first, then I can rebase this one.

Made the necessary changes to get this example to compile. This includes
replacing the ".." placeholders with comments. I also changed the
formatting of the `else if` block in `read`, to make it easier to
include a placeholder comment in there.
Fix some problems in the `Mutex` example. Please note that it still
doesn't compile, as it's obviously rather abstract and incomplete, in
order to demonstrate the principle.
Please note that it still doesn't compile, partly because parts of it
are pseudo-code, partly because of futures-related issues that I don't
understand, and don't currently have the time to get into.
It still doesn't compile, due to the fake `Mutex` and `CircularBuffer`
types, as well as the ".." placeholders, but I've fixed the other
errors.
@japaric
Copy link
Member

japaric commented Sep 22, 2017

I'll review this after #19 lands.

@hannobraun
Copy link
Member Author

Hmm, this is a bit of a mess. Even though I've kept this up to date with the changes in #19, this pull request is still showing 9 commit instead of the 2 that actually differ. "Files changed" also shows the changes from #19, even though those have been merged.

I don't know how to update it, but I guess it's not a big problem. Only the last two commits are relevant. The rest of them have already been merged into master.

@hannobraun
Copy link
Member Author

@japaric Is there anything I can do to help this along? Do you want me to open a new pull request for this, to get rid of those commits that were already merged om #19?

@japaric
Copy link
Member

japaric commented Oct 17, 2017

Sorry I had forgotten about this PR. It LGTM now.

@homunkulus r+

@homunkulus
Copy link
Contributor

📌 Commit 5e2c733 has been approved by japaric

@homunkulus
Copy link
Contributor

⌛ Testing commit 5e2c733 with merge 5aa6d09...

japaric pushed a commit that referenced this pull request Oct 17, 2017
Change traits to take `&mut self`, where appropriate

This pull request supersedes #16, which seems stalled. It is based on #19. Only the last two commits are actually relevant to this pull request itself. I suggest merging #19 first, then I can rebase this one.
@homunkulus
Copy link
Contributor

☀️ Test successful - status-travis
Approved by: japaric
Pushing 5aa6d09 to master...

@homunkulus homunkulus merged commit 5e2c733 into rust-embedded:master Oct 17, 2017
@hannobraun hannobraun deleted the mut-self branch October 17, 2017 18:30
@hannobraun
Copy link
Member Author

No trouble. Thank you!

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.

3 participants