rust-lexical
rust-lexical copied to clipboard
[OTHER] Improve internal safety comments and architecture
I was reviewing the lexical-write-integer crate and I discovered a bunch of things that could be improved wrt safety.
The main issue was the one already filed as https://github.com/Alexhuszagh/rust-lexical/issues/95. That's actual UB, this issue is more about the safety comments and how the invariants are upheld. The safe
mode doesn't disable this code that has actual UB, either.
Besides the issue that already exists, there are a whole bunch of functions (e.g. write_digits()
) that unsafely index from table
are documented with "This is safe as long as the buffer is large enough to hold T::MAX
digits in radix N
.",but they do not include the safety requirement on
table`.
Furthermore, some functions (also write_digits()
) take a mut index: usize
argument that is used to directly index. These safety requirements are also not mentioned.
Besides safety comments, I found a couple areas where it was hard to review the unsafe code:
A bunch of code relies on the radix being from amongst a valid set (e.g. get_table()
). May benefit from being an enum; it's hard to tell if this invariant is being upheld.
Other code which is calling the write_*()
functions (e.g. unsigned()
) declare they are safe as long as the buffer can hold FORMATTED_SIZE
elements, but that invariant is not really clear from the functions used, it's kinda hidden away.
Might be cool to have a ValidBuffer<FORMAT>
wrapper that allows calling these write functions in an encapsulated way.
Anyway, hope this helps. I don't have time to devote to doing this work myself but I figured I'd share my notes in case someone else does.