Added comments when using unsafe#1501
Added comments when using unsafe#1501danieldyn wants to merge 1 commit intodimforge:mainfrom danieldyn:explain-unsafe-usage
Conversation
| /// | ||
| /// The floating-point value's bit-level representation is preserved. | ||
| /// | ||
| /// Using unsafe is sound because the bitwise representation of f32 fits in i32 |
There was a problem hiding this comment.
This documentation goes inside the function, on the unsafe block. The use of unsafety in the implementation is not public; the purpose of the comment is to help a developer or auditor who is looking at the implementation.
| /// | ||
| /// The floating-point value's bit-level representation is preserved. | ||
| /// | ||
| /// Using unsafe is sound because the bitwise representation of f32 fits in i32 |
| lapack_check!(info); | ||
|
|
||
| // Copy lower triangle to upper triangle. | ||
| // Using unsafe to ensure the bounds i and j are always valid indices, |
There was a problem hiding this comment.
This comment doesn't make sense. The unsafe operation requires that the indices are in bounds; it does nothing to ensure that is actually the case. The comment should explain how we know it's the case.
| } | ||
|
|
||
| /// This macro uses unsafe to manually ensure memory safety for external functions | ||
| /// For incorrectly sized and initialized matrices and arrays, undefined behavior will occur |
There was a problem hiding this comment.
This comment seems to be arguing that the use of unsafety is incorrect. If that's true, then we should fix the unsoundness, not just declare its presence. Several similar looking instances below.
|
|
||
|
|
||
| // Using unsafe to manually ensure memory safety for external function lapack_func | ||
| // Rust cannot check the slices' or raw poinnters' safety |
There was a problem hiding this comment.
Here and below, the comment must explain exactly what the safety invariants are, and how we know they're satisfied.
What changed?
unsafeis sound #628Why?
To get issue #628 closer to being completely solved