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

make alloc and collections compilable for thumbv6m-none-eabi #37492

Merged
merged 3 commits into from
Dec 9, 2016

Conversation

japaric
Copy link
Member

@japaric japaric commented Oct 31, 2016

by cfging away alloc::Arc and changing OOM to abort for this target

r? @alexcrichton
cc @thejpster

by cfging away `alloc::Arc` and changing OOM to abort for this target
@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (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.

#[inline(never)]
#[unstable(feature = "oom", reason = "not a scrutinized interface",
issue = "27700")]
pub fn oom() -> ! {
Copy link
Member

Choose a reason for hiding this comment

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

To help with the #[cfg] amount in this module, perhaps this could have two mod imp modules with the #[cfg] attributes and the implementation is reexported?

@alexcrichton
Copy link
Member

cc @rust-lang/libs, perhaps more input for the scenario discussion

@whitequark
Copy link
Member

+1 from me; our LLVM target (OR1K) did not have atomics nor our use case had any need for atomics. I have since partially implemented atomics, which was rather tricky due to issues with OR1K ISA, but should probably not have bothered, and this functionality would have been welcome.

@alexcrichton alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Nov 10, 2016
@alexcrichton
Copy link
Member

@japaric looks like not too many other opinions from @rust-lang/libs so let's move forward with this, want to update the oom.rs with my suggestion to cut down on #[cfg] annotations and I'll r+?

@brson
Copy link
Contributor

brson commented Nov 11, 2016

I'd like more time to think about this.

There are a lot of pieces of std that people want to start slicing out but so far there are very few places where we use cfg to remove significant APIs. I don't want to initiate such a policy on a whim.

@japaric
Copy link
Member Author

japaric commented Nov 11, 2016

want to update the oom.rs with my suggestion to cut down on #[cfg] annotations

Done.

@alexcrichton
Copy link
Member

@brson ah yeah sure. To clarify my thinking, it's always been my impression that std gives you everything it can when compiled for any particular platform. That is, it just doesn't compile in anything it can't support. The usage of scenarios eventually would then provide nice gated access to all APIs and a principled way to work with the standard library.

@alexcrichton
Copy link
Member

r? @brson

@rust-highfive rust-highfive assigned brson and unassigned alexcrichton Nov 11, 2016
@japaric
Copy link
Member Author

japaric commented Nov 18, 2016

@brson

There are a lot of pieces of std that people want to start slicing out but so far there are very few places where we use cfg to remove significant APIs. I don't want to initiate such a policy on a whim.

I want to note that:

  • This is cfg-ing away a chunk of the alloc crate, not std's
  • These changes can't make it to std because std won't compile for a target that has max-atomic-width = 0 even without this change.
  • The only people that will observe that some API is missing from it are the people that are directly using the alloc crate.
  • The alloc crate is unstable. We can change how the API in it is presented to users (cfg, scenarios, lints, etc.) at any time and break programs.

@japaric
Copy link
Member Author

japaric commented Dec 7, 2016

@brson ping! Saw my last comment? ^

@brson
Copy link
Contributor

brson commented Dec 9, 2016

@bors r+

@bors
Copy link
Contributor

bors commented Dec 9, 2016

📌 Commit abe6fc7 has been approved by brson

@bors
Copy link
Contributor

bors commented Dec 9, 2016

⌛ Testing commit abe6fc7 with merge 6a495f7...

bors added a commit that referenced this pull request Dec 9, 2016
make `alloc` and `collections` compilable for thumbv6m-none-eabi

by cfging away `alloc::Arc` and changing OOM to abort for this target

r? @alexcrichton
cc @thejpster
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants