-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Add support for padding in snprintf #30
Conversation
Implementation looks good - just a couple of typos. I think I'd prefer to see the test cases split out, as it can be quite hard to see which output corresponds to which input, and the right-padding on one output runs into the left-padding on the next. Perhaps: /// Make it easier to turn `c"Hello"` into a `*const CChar`
trait ToByte {
fn cp(&self) -> *const CChar;
}
impl ToByte for &std::ffi::CStr {
fn cp(&self) -> *const CChar {
self.as_ptr().cast()
}
}
/// Handle the buffer that `snprintf` needs
fn asprintf<F>(f: F) -> String
where
F: FnOnce(*mut CChar, usize) -> i32,
{
let mut buf = vec![0u8; 128];
let res = f(buf.as_mut_ptr(), buf.len());
if res < 0 {
panic!("closure returned {}", res);
}
// res does not include the trailing NUL that snprintf always outputs (if there's space)
buf.truncate((res + 1) as usize);
let cs = std::ffi::CString::from_vec_with_nul(buf)
.expect("failed to make CString from closure output");
cs.to_str().expect("was not UTF-8").to_string()
}
#[test]
fn plain_string() {
assert_eq!(
"Hi",
asprintf(|buf, len| unsafe { snprintf(buf, len, c"Hi".cp()) })
);
} |
The old test cases would look like: #[test]
fn numbers() {
assert_eq!(
"100",
asprintf(|buf, len| unsafe { snprintf(buf, len, c"%u".cp(), CUInt::from(100u8)) })
);
assert_eq!(
"100",
asprintf(|buf, len| unsafe { snprintf(buf, len, c"%lu".cp(), CULong::from(100u8)) })
);
assert_eq!(
"100",
asprintf(|buf, len| unsafe {
snprintf(buf, len, c"%llu".cp(), CULongLong::from(100u8))
})
);
assert_eq!(
"-100",
asprintf(|buf, len| unsafe { snprintf(buf, len, c"%d".cp(), CInt::from(-100i8)) })
);
assert_eq!(
"-100",
asprintf(|buf, len| unsafe { snprintf(buf, len, c"%ld".cp(), CLong::from(-100i8)) })
);
assert_eq!(
"-100",
asprintf(|buf, len| unsafe {
snprintf(buf, len, c"%lld".cp(), CLongLong::from(-100i8))
})
);
assert_eq!(
"cafe1234",
asprintf(|buf, len| unsafe {
snprintf(buf, len, c"%x".cp(), CUInt::from(0xcafe1234u32))
})
);
assert_eq!(
"cafe1234",
asprintf(|buf, len| unsafe {
snprintf(buf, len, c"%lx".cp(), CULong::from(0xcafe1234u32))
})
);
assert_eq!(
"CAFE1234",
asprintf(|buf, len| unsafe {
snprintf(buf, len, c"%llX".cp(), CULongLong::from(0xcafe1234u32))
})
); |
(curse rustfmt for breaking some lines but not others) |
Good idea. I was trying to simplify the tests, too. But I failed because of the variable argument count which Rust does not support. I will rewrite the tests like that. |
Agreed, but if the assert panics you'll get a line number from inside asprintf, as opposed to the line number of the test case. |
I tried to make it as readable as possible, but if you don't like that fact that the line number is wrong, I can leave the asserts in the test cases instead. |
I found a solution for the test case line numbers. With |
Rust 1.75 did not support c-string literals. |
C-Strings are really useful so I'm happy to bump the MSRV and if someone wants to submit a patch to put it back again they are welcome to. |
Have we, or can we, run these test cases against newlib or some other C Standard Library to check we got them all right? Perhaps I should write a C program we can link against both a real C library and this C library to check they match. That's OK, I'll do another PR for that. Maybe there's an official C Standard Library test suite or something. |
|
That way the snprintf test cases can be checked against libc by running `cargo test --no-default-features`
Good idea. I removed the feature gate and the tests succeed with and without I don't know much about Github workflows though, so the automation should probably be done in a separate PR by you. |
Thank you for this PR! |
4f6da13
This could also be under some feature flag, to reduce code size of required.
While I was at it I also tried to simplify the handling of different integer sizes.