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

sys/string_utils: add string_writer helper #20843

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

benpicco
Copy link
Contributor

Contribution description

Assembling a string through multiple writes if often open-coded like this

char buffer[64];
size_t len = 0;
for (unsigned i = 0; i < ARRAY_SIZE(names); ++i) {
    len += snprintf(&buffer[len], sizeof(buffer) - len, "%u: %s\n", i, names[i]);
}

This is error prone (the above example foregoes any error handling) and ugly.

This introduces a small helper function to achieve the same in a much more convenient fasion:

char buffer[64];
string_writer_t sw;
string_writer_init(&sw, buffer, sizeof(buffer));

for (unsigned i = 0; i < ARRAY_SIZE(names); ++i) {
    swprintf(&sw, "%u: %s\n", i, names[i]);
}

Testing procedure

A new unit test has been added.

Issues/PRs references

@benpicco benpicco requested a review from miri64 as a code owner August 29, 2024 12:04
@github-actions github-actions bot added Area: tests Area: tests and testing framework Area: sys Area: System labels Aug 29, 2024
@maribu
Copy link
Member

maribu commented Aug 29, 2024

That API indeed disarms the usual footguns :)

I wonder if there is some BSD / whatever API already that we could steal?

sys/include/string_utils.h Outdated Show resolved Hide resolved
Copy link
Contributor

@Teufelchen1 Teufelchen1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All in all looks good. Some nits, some ideas.

You mentioned that this is often done manually in RIOT, could you also replace such instances with this helper right away?

tests/unittests/tests-libc/tests-libc.c Outdated Show resolved Hide resolved
Comment on lines 46 to 60
res = swprintf(&sw, "0123456789");
TEST_ASSERT_EQUAL_INT(res, 10);
res = swprintf(&sw, "01234567891");
TEST_ASSERT_EQUAL_INT(res, -E2BIG);
res = swprintf(&sw, "###");
TEST_ASSERT_EQUAL_INT(res, -E2BIG);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is worth it, to use a new string_writer_t + buffer. Not because it's technically needed but it signals to the reader "we are testing a new/different behavior". Also, which behavior is tested here? ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No this is all the same string write - we want to append to it until it's full and test at every step that it behaves as expected.

I added comments to better explain what's going on.

tests/unittests/tests-libc/tests-libc.c Show resolved Hide resolved
sys/libc/string.c Show resolved Hide resolved
sys/include/string_utils.h Outdated Show resolved Hide resolved
sys/include/string_utils.h Outdated Show resolved Hide resolved
sys/include/string_utils.h Outdated Show resolved Hide resolved
Copy link
Contributor

@Teufelchen1 Teufelchen1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great improvements.

tests/unittests/tests-libc/tests-libc.c Show resolved Hide resolved
tests/unittests/tests-libc/tests-libc.c Show resolved Hide resolved
tests/unittests/tests-libc/tests-libc.c Outdated Show resolved Hide resolved
@Teufelchen1
Copy link
Contributor

Squash & Ci?

@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Sep 10, 2024
@riot-ci
Copy link

riot-ci commented Sep 10, 2024

Murdock results

FAILED

0fe3ac4 unittests: add test for string_writer

Success Failures Total Runtime
28 0 9199 01m:33s

Artifacts

* @return number of bytes written on success
* -E2BIG if the string was truncated
*/
int swprintf(string_writer_t *sw, const char *restrict format, ...);
Copy link
Contributor

@derMihai derMihai Sep 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can add

__attribute__ ((format (printf, 2, 3)))

to enable static printf-like compiler checks for the format string and arguments, see https://gcc.gnu.org/onlinedocs/gcc-3.2/gcc/Function-Attributes.html.

#endif

va_start(args, format);
res = vsnprintf(sw->position, sw->capacity, format, args);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang should complain here, you can:

#ifdef __clang__
    _Pragma("clang diagnostic push") \
    _Pragma("clang diagnostic ignored \"-Wformat-nonliteral\"") \
    res = vsnprintf(sw->position, sw->capacity, format, args);
    _Pragma("clang diagnostic pop")
#else
    res = vsnprintf(sw->position, sw->capacity, format, args);
#endif

res = vsnprintf(sw->position, sw->capacity, format, args);
va_end(args);

if (res <= (int)sw->capacity) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if res == capacity, the output was truncated by one character. This check should therefore be <.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: sys Area: System Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants