Skip to content
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

Merged
merged 8 commits into from
Oct 28, 2024

Conversation

Sympatron
Copy link
Contributor

@Sympatron Sympatron commented Oct 23, 2024

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.

src/snprintf.c Outdated Show resolved Hide resolved
src/snprintf.c Outdated Show resolved Hide resolved
@jonathanpallant
Copy link
Collaborator

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()) })
	);
}

@jonathanpallant
Copy link
Collaborator

jonathanpallant commented Oct 25, 2024

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))
			})
		);

@jonathanpallant
Copy link
Collaborator

(curse rustfmt for breaking some lines but not others)

@Sympatron
Copy link
Contributor Author

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.
It could be even shorter/more readable if asprint took the expected string and compared it, too.

@jonathanpallant
Copy link
Collaborator

It could be even shorter/more readable if...

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.

@Sympatron
Copy link
Contributor Author

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.

@Sympatron
Copy link
Contributor Author

I found a solution for the test case line numbers. With #[track_caller] on asprintf it is skipped in stack traces. That way it prints the line number of the invocation of asprintf instead.

@Sympatron
Copy link
Contributor Author

Rust 1.75 did not support c-string literals.
Do you want to set use clippy >= 1.77 or should I manually terminate the strings?

@thejpster
Copy link
Member

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.

@thejpster
Copy link
Member

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.

@thejpster
Copy link
Member

cargo test --no-default-features should do it, as that will stop snprintf.c being compiled, and so we should use the version from libc instead. You will have to take the feature gate off the test module though to ensure the tests still run.

That way the snprintf test cases can be checked against libc by running `cargo test --no-default-features`
@Sympatron
Copy link
Contributor Author

cargo test --no-default-features should do it, as that will stop snprintf.c being compiled

Good idea. I removed the feature gate and the tests succeed with and without --no-default-features. It does not seem to work on Windows, though. At least on my machine snprintf is not found by the linker. But I don't think that is a problem, because it is only required to work in automated tests on any machine for reference.

I don't know much about Github workflows though, so the automation should probably be done in a separate PR by you.

@thejpster
Copy link
Member

Thank you for this PR!

@thejpster thejpster added this pull request to the merge queue Oct 28, 2024
Merged via the queue into rust-embedded-community:master with commit 4f6da13 Oct 28, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants