-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in 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.