Code review comment for lp:~jamesodhunt/libnih/fix-for-bug-834813

> "str && *str" shouldn't be necessary, this function already asserts "str !=
> NULL"
>
> Have you checked other uses of strchr() in the code, I probably make the same
> mistake all over the place - believing it would return NULL if given '\0'
>
> I'd like that final if() cleaned up, it doesn't make sense what you're doing
> there - perhaps splitting out some informationally named temporary variables
> and checking those would make more sense, or using some other method
Hi Scott,

Firstly, I've raised a bug on strchr(3) and memchr(3) since they do not currently specify the behaviour should the char to search for be '\0':

https://bugzilla.kernel.org/show_bug.cgi?id=42042

I've reworked (simplified) the change to nih_str_split(). Note that the "continue" is safe since we consume any and all multiple delimiter characters on the next loop iteration.

The assert protects the function initially, but since we're incrementing 'str' we do need atleast the first "str && *str" check...

   while (repeat && str && *str && strchr (delim, *str))

...since without it, we overrun the string, and the new tests will fail. The subsequent "str && *str"'s may not be strictly required, but as this bug was rather subtle anyway, I'd be tempted to err on the side of caution.

I'll resubmit the MP...

Cheers,

James.