Tweak util_libc::open_readonly
#434
Conversation
| /// If `path` does not contain any zeros. | ||
| #[inline(always)] | ||
| pub fn open_readonly(path: &[u8]) -> Result<libc::c_int, Error> { | ||
| assert!(path.iter().any(|&b| b == 0)); |
There was a problem hiding this comment.
OK, but since this already returns a Result, we should avoid assert!. Either the compiler is going to optimize away the check completely anyway, or we'd be left with dead code that constructs an Err, or we'd be left with dead code that panics. The possibility of dead code that panics is problematic for some static analysis mechanisms that try to ensure there are no panics in a program.
There was a problem hiding this comment.
I think having an assert! here is fine. If this ever triggers, it's an error in our code, not an error in the user's calling environment or input.
For opt-level=1 (https://rust.godbolt.org/z/K3cjrsxqa) or opt-level=s (https://rust.godbolt.org/z/MT9jozE1e) , the assert is optimized away, and on opt-level=0 there's a bunch of dead code that panics, even if the assert is replaced with a branch(https://rust.godbolt.org/z/939exhnKn). Leaving it as-is also makes things more readable/debuggable.
A better approach for this is documenting and testing that our implementation does not panic, I opened #435.
| /// If `path` does not contain any zeros. | ||
| #[inline(always)] | ||
| pub fn open_readonly(path: &[u8]) -> Result<libc::c_int, Error> { | ||
| assert!(path.iter().any(|&b| b == 0)); |
There was a problem hiding this comment.
I think having an assert! here is fine. If this ever triggers, it's an error in our code, not an error in the user's calling environment or input.
For opt-level=1 (https://rust.godbolt.org/z/K3cjrsxqa) or opt-level=s (https://rust.godbolt.org/z/MT9jozE1e) , the assert is optimized away, and on opt-level=0 there's a bunch of dead code that panics, even if the assert is replaced with a branch(https://rust.godbolt.org/z/939exhnKn). Leaving it as-is also makes things more readable/debuggable.
A better approach for this is documenting and testing that our implementation does not panic, I opened #435.
Based on discussion in #432,
open_readonlynow accepts&[u8]instead of&strfor path and asserts that it contains at least one zero byte. This allows to make the function safe and compiler should be able to remove the assert since we use static paths.