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

linked_list_allocator update for AllocRef and spinning_top #26

Merged
merged 1 commit into from
Jun 5, 2020

Conversation

sbechet
Copy link
Contributor

@sbechet sbechet commented Feb 19, 2020

See #25

@sbechet sbechet requested a review from a team as a code owner February 19, 2020 12:46
@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @korken89 (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

Copy link
Contributor

@jonas-schievink jonas-schievink left a comment

Choose a reason for hiding this comment

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

It seems that #25 is not actually an issue that needs fixing, since it only needs a patch version bump. Changes look good to me though.

bors r+

@bors
Copy link
Contributor

bors bot commented Feb 19, 2020

Canceled

@sbechet
Copy link
Contributor Author

sbechet commented Feb 19, 2020

I don't know why the two pull request come to only one. A friend help me for #27

@sbechet
Copy link
Contributor Author

sbechet commented Mar 1, 2020

what's wrong with bors?

@kanishkarj
Copy link

Hey, I am facing this issue too, I guess the update to linked_list_allocator is kind of necessary..

@sbechet
Copy link
Contributor Author

sbechet commented May 10, 2020

Where are you code owner :)
Is this code so bad?

@therealprof
Copy link

@sbechet Would you mind rebasing/squashing this? The history is a bit messy.
@jonas-schievink Is the CS okay to use here?

@jonas-schievink
Copy link
Contributor

@therealprof We were already using a CS so that's fine.

@sbechet This PR needs to be synced against master, other PRs have landed in the mean time and this crate seems to build fine on nightly now. If this is still needed, please explain why and what it does.

@sbechet
Copy link
Contributor Author

sbechet commented May 10, 2020

not sure how to rebase cleanly to clean history. Must i rebase then force remote push on my own repository ?

@sbechet sbechet force-pushed the master branch 4 times, most recently from 4a68e81 to 36366c9 Compare May 10, 2020 15:08
@sbechet
Copy link
Contributor Author

sbechet commented May 10, 2020

Hello Jonas and Daniel,

I think it's ok now.

  • synced against master
  • cleaned history

@MathiasKoch
Copy link

Any progress on this?

@jonas-schievink
Copy link
Contributor

r=me with rebase

@sbechet
Copy link
Contributor Author

sbechet commented May 27, 2020

done again
ping @jonas-schievink

@jonas-schievink
Copy link
Contributor

This is failing in CI, and it seems like you've included some commits that are on master already

@sbechet
Copy link
Contributor Author

sbechet commented Jun 4, 2020

I don't know what is the best solution between this one and that of bugadani. Like you want.

Copy link
Contributor

@jonas-schievink jonas-schievink left a comment

Choose a reason for hiding this comment

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

bors r+

@bors
Copy link
Contributor

bors bot commented Jun 5, 2020

Build succeeded:

@bors bors bot merged commit 3f0925f into rust-embedded:master Jun 5, 2020
@bugadani
Copy link

bugadani commented Jun 5, 2020

I admit, this is much nicer on the code level than #33. However, since RefCell is not thread safe, and incurs a runtime penalty, I'm not sure if merging this was the right move on the long run. But that's just my 0.02$, I'm glad the crate compiles anyway :)

@jonas-schievink
Copy link
Contributor

Mutex makes it thread-safe, but you're right about the runtime overhead.

Since having an allocator incurs overhead anyways and implies that you have plenty of RAM and can spare the cycles, I think adding another byte of overhead is fine here (and I don't really want to deal with more unsafety than necessary).

@bugadani
Copy link

bugadani commented Jun 5, 2020

Mutex makes it thread-safe

I'm not here to start an argument, but there's a note in bare-metal which disagrees. /// **This Mutex is only safe on single-core systems.**

@jonas-schievink
Copy link
Contributor

Ah, that is outdated due to RFC 419

@bugadani
Copy link

bugadani commented Jun 5, 2020

I see. Thank you :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants