Add no_panic feature#28
Conversation
|
I'm using this crate and producing some wasm file, when this feature is enabled, the size of the wasm file is reduced. |
There was a problem hiding this comment.
Sorry for the massive delay: for some reason I didn't get any notifications from Github for a period. My email is in my Github bio for whatever reason in case I stop getting Github emails: feel free to reach out to me directly there as well.
The general approach looks good, I do have a few changes I'd like to see, specifically to ensure safety invariants are guaranteed (or, in no-panic mode, if they are not).
Security issues have been an issue: when I took over this fork, there was a lot of non-local safety invariants and also some undefined behavior that was fixed. So although everything is sound here, I'd much rather these be very clearly documented, both for source maintainers and for end-users, the panic behaviors.
| pub fn parse_decimal(mut s: &[u8]) -> Decimal { | ||
| // can't fail since it follows a call to parse_number | ||
| assert!(!s.is_empty(), "the buffer cannot be empty since it follows a call to parse_number"); | ||
| debug_assert!( |
There was a problem hiding this comment.
I feel this should be an assert in normal mode (it will be optimized out) and only debug_assert in no-panic mode. I like my strong safety invariants by default. Maybe create a shim for the macro? nopanic_assert?
| T::parse_float_partial(s) | ||
| } | ||
|
|
||
| pub trait GetAt<Index, R: ?Sized> { |
There was a problem hiding this comment.
I don't think this should be exposed as part of the public API and should be clearly documented to use unsafe code in no-panic mode.
|
|
||
| ## Features | ||
|
|
||
| `no-panic`: when this feature is enabled, the crate guarantees that it will not trigger a runtime panic. |
There was a problem hiding this comment.
This should clearly document that unsafe code is used to this effect.
2 participants
No description provided.