-
Notifications
You must be signed in to change notification settings - Fork 186
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
API alignment #442
Comments
I agree with this. It's going to be an annoying breaking change, but There are definitely other places where the API diverges, In general we're going to have the same problems the Linux kernel has with |
I'm not sure if I agree with this. It's true std's In std, you treat RAM as a practically-infinite single pool. When it does fill up, your program's state is thoroughly hosed because any other allocation will also fail, and pretty much everything in std rust does allocations. The kernel handles OOM terribly too, it can hose your entire machine, not just your process. So panicking in std vec makes sense. In haepless, it's much more likely to run into so IMO crashing on heapless vec full is never the right choice (except in extremely quick-and-dirty firmware prototypes). |
I think it's important to keep in mind that we're not so much talking about an overall functionality change, but more about naming. For the case of
For many cases, |
People will use the more "obvious" method named You mention ease of porting. However to port code to heapless well you want to handle "full" errors. If heapless |
I don't think a panicing API can be that easily disregarded as "wrong". For example, indexing a slice (which is also the obvious approach to retrieving an item) would then be wrong as well. It's just as easy to crash! And with your line of argument, you'd need to convert indexing operations into It's clear to every user of heapless that moving from And the porting consideration should not be focused on |
Slice indexes are usually controlled by the "algorithm" your code is implementing, Index out of range panics are almost always bugs in your code, ie "your" fault. Heapless vec push full errors are usually controlled by "the environment": number of records from deserializing a network message, number of button presses the user did in the PIN code keypad... It's common for your code to be entirely correct and still run into "full" errors, due to none of your fault, just because the environment you're interacting with fed you too much data.
Sure reasoning is the same, but making
A crate having many downloads doesn't automatically mean all API decisions they made are correct. Also, I don't think "I want .push() that panics" is what's getting people to use arrayvec instead of heapless. |
I'd be very surprised if that statement (that indices are not user/external data while push() traffic is triggered by user/external data) holds water. There is nothing in the code or the docs that supports this AFAICT. And it's trivial to come up with plenty of counter-examples. |
pretty much every single protocol does "send N things" or "send a variable-length string", while it's relatively rare to send indices. |
Come on! "rare"? When receiving said data, you'll use some length data passed to you to index or |
Indices used by the indexing operator lengths do come from the network. "read |
You misread me. I was referring to indexing a receive buffer or splitting it at an index. This is both common and panicing. Hence a trivial counterexample to your claim that indices are trusted. In any case we've strayed far away from the original point I was making: that it may be preferable to stick with the API of |
Another question regarding aligning the API with |
I disagree with the idea of aligning APIs for the purpose of the APIs being the same, when the context in which this crates is used is wildly different from The reason that In a lot of cases, users will need to put arbitrary limit on the data stored in the In the case of heapless on the other hand, users know that the To give a practical example, in our case at Nitrokey, we would be using |
Given that we have a bunch of types that don't have an equivalent in For example, if Also, currently the fallible version of |
Exactly. The context is different and there needs to be a fallible op. But the context doesn't demand
Presumably you need to do that audit anyway and (hopefully) already do it for all the other panics ( A compromise could be to (just) rename the current
Sure. And other places as well.
Right. The name would be |
Just renaming |
The reason for naming thing |
There are |
I didn't even know that I wonder if it wouldn't be an improvement to actually implement it on |
In the light of aiming for a stabilized API of v1.0 (#410) but having just done a significant API changeset in 0.8, I wanted to bring something up that has been bugging me several times.
Currently the
heapless::Vec
API is significantly different from bothstd::Vec
and alternative no_std/no-allocVec
implementations liketinyvec::ArrayVec
orarrayvec::ArrayVec
or coca. This is especially true for the important case wherestd::Vec
would reallocate but its fixed size alternatives must either panic or become fallible: e.g.Vec::push()
.heapless
deviates from the others in its API and makes these fallible. The others are panicing and additionally/alternatively tend to offer a fallible API (ArrayVec::try_push()
).IMO it would be nice if we could overall
std::Vec
API andtinyvec
try_push()
etc).That would make porting a bit easier. And it could allow
heapless::Vec
to be a drop-in replacement fortinyvec::ArrayVec
. The only (AFAICT) remaining difference would be under the hood: the use ofMaybeUninit
.The API difference comes up from time tom time e.g. #440.
There is demand for distinct operations that are both infallible and non-panicing: #341
There may also be other places in
heapless
where the API choice diverges from thestd
analogue.The text was updated successfully, but these errors were encountered: