Check allocated pointers before using them
It's good that we have a if (!temp)
check, but unfortunately we used temp
as argument to memset()
before we did the check, so it's too late!
We could (and should) use calloc()
if we need to zero out the memory we allocated. In this case, it's not necessary (we'll be overwriting it anyway), so we should just omit the memset()
call.
Use const
where appropriate
The pointers str
, p
and q
can all point to const char
; that allows us to safely pass a string literal as argument, for example, and it helps users understand what guarantees they are given.
Document the arguments
The meaning of the argument len
isn't obvious from its name or from the function's comment. It seems that it's the largest substring we expect to handle.
Even better would be to make the code adapt (using realloc()
, perhaps) to the string it's given, rather than requiring the user to specify that. I'll leave that as an interesting exercise for you.
Try not to mix indexing and pointer arithmetic
It's a little confusing to index into str
using i
, but to also have pointers p
and q
pointing into the string, too.
The break
at the end of the loop
Since p
initially points to the beginning of the string, we can move the test at the end of the loop to be part of the loop condition. This makes the loop structure easier to follow:
for (size_t i = 0; i <= len && *p; ++i)
(I changed the test of i
slightly, for simplicity and also as an almost-automatic aversion to integer overflow).
We don't need a loop to find the next instance of token
We could use strchr()
to find the next token
in str
. On GNU platforms, there's also strchrnul()
which returns a pointer to the terminating NUL (instead of a null pointer) when the character isn't present.
Consider discarding empty words
Many users will expect the same result from "foo bar"
and "foo bar"
- it's a surprise to find that this code considers there to be a zero-length word between the two spaces in the latter string. Even more surprising is " foo"
which splits to ""
and " foo"
.
Reworked code
I started again from scratch, taking into account these observations. The interface is a bit different, as in real life we want to be able to re-use the function to perform different actions on the tokens found, so we accept a function and a state pointer for the function.
This is standard portable C - you're of course able to add your annotation header to this if you want.
#include <stdlib.h>
#include <string.h>
/**
* @brief Call a function for each token found in the input string.
* @param s input string
* @param sep nul-terminated set of delimiter characters, or NULL for default
* @param f callback function
* @param data callback data
* @return true if successful, false on any failure
*/
int process_tokens(const char *s, const char *sep,
void (*f)(const char*,void*), void *data)
{
if (!sep) {
/* use default word separators */
sep = " \t\n.!?()";
}
char *buf = NULL;
size_t buf_len = 0;
while (*(s+= strspn(s, sep))) {
/* s is now pointing at next word character */
size_t word_len = strcspn(s, sep);
/* ensure we have sufficient storage */
if (buf_len <= word_len) {
buf_len = 2 * word_len + 1;
char *t = realloc(buf, buf_len);
if (!t) {
free(buf);
return 0;
}
buf = t;
}
/* copy word into storage, and call user function */
memcpy(buf, s, word_len);
buf[word_len] = 0;
f(buf, data);
/* advance to next separator */
s += word_len;
}
free(buf);
return 1;
}
/* A simple test program */
#include <stdio.h>
static void print_token(const char *s, void *data)
{
size_t *count = data;
++*count;
printf("Processed: \"%s\"\n", s);
}
int main()
{
size_t count = 0;
const char *input = " Pack my box with five-dozen liquor jugs. ";
if (!process_tokens(input, NULL, print_token, &count)) {
fprintf(stderr, "Error splitting input string\n");
return 1;
}
printf("Processed %zu tokens\n", count);
}
<sal.h>
? That's not a standard library header - does it belong to some library? I'm guessing, given__cdecl
that it might be winapi or the like? \$\endgroup\$