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

Style for optional memory allocation in no_std #151

Closed
sffc opened this issue Jun 25, 2020 · 5 comments
Closed

Style for optional memory allocation in no_std #151

sffc opened this issue Jun 25, 2020 · 5 comments
Labels
A-design Area: Architecture or design C-meta Component: Relating to ICU4X as a whole question Unresolved questions; type unclear

Comments

@sffc
Copy link
Member

sffc commented Jun 25, 2020

When discussing no_std (#77), we decided to shoot for no_std without alloc. I wanted to follow up on one aspect of that decision. It is very common in ICU code for the majority of operations to be performed without an allocation, but allocation may be necessary in edge cases. Therefore, when writing ICU4X code that works with no_std in 95% of cases, how should we handle cases when an allocation is required?

  1. Panic
  2. Ignore (e.g., don't push another element to a vector if it is full; safe, but undefined behavior)
  3. Return error result
  4. Conditionalize any such API on the std feature
  5. Require the alloc trait

Option 1 is the easiest, but may be hostile to no_std users, since they can't know ahead of time whether or not their call will panic, and panicking is not recoverable. To a certain extent, we could rely on documentation for what edge cases trigger panics, but we risk the documentation getting stale.

Option 2 is also easy, but maybe we don't want to have undefined behavior.

Option 3 is harder, because it requires instrumenting our code up and down the stack to have fallible memory allocation, probably with clippy warnings to enforce it. However, this is ICU4C's style, so maybe it's not out-of -line for ICU4X.

Option 4 would be an unfortunate conclusion, because a large majority of ICU4X APIs will require allocation in at least some edge cases.

Option 5 was shot down by multiple people in #77.

@Manishearth @hsivonen @zbraniecki

@sffc sffc added question Unresolved questions; type unclear C-meta Component: Relating to ICU4X as a whole A-design Area: Architecture or design labels Jun 25, 2020
@Manishearth
Copy link
Member

I think we can start with Option 4 and add fallible versions of APIs to get to option 3 as we need them?

@sffc
Copy link
Member Author

sffc commented Jun 25, 2020

Okay. I'm clearly still not aligned with @Manishearth's line of thinking when it comes to no_std. 😕

Option 4 in effect means that we simply do not support no_std in most of ICU4X, and consider it a nice-to-have for some undetermined point in the future, with no clear roadmap to get there. That's not my vision, but maybe I should just give up on the advocacy for no_std.

@Manishearth
Copy link
Member

@sffc I mean, the idea is to not make it a requirement to get started. Being able to add this stuff incrementally is quite good.

@zbraniecki
Copy link
Member

Option 4 in effect means that we simply do not support no_std in most of ICU4X, and consider it a nice-to-have for some undetermined point in the future, with no clear roadmap to get there. That's not my vision, but maybe I should just give up on the advocacy for no_std.

That's not how I read it Shane. I read it as "let's get the right API working and then investigate how much of it we can tweak for no_std".
The way I read your position is that we need to aggressively enforce no_std today or otherwise it'll be a lot of work to do it later.

This worry is not my experience in Rust and a lot of projects add no_std feature as they mature. I think Manish is suggesting we do the same here.

@sffc
Copy link
Member Author

sffc commented Jun 25, 2020

Okay. I'm documenting the current decision in #154 and will close this issue.

@sffc sffc closed this as completed Jun 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-design Area: Architecture or design C-meta Component: Relating to ICU4X as a whole question Unresolved questions; type unclear
Projects
None yet
Development

No branches or pull requests

3 participants