[Mesa-dev] [PATCH v6 1/3] util: Add util_strlcpy

[Gustaw Smolarczyk]

2017-07-06 20:52 GMT+02:00 Robert Foss <robert.foss at collabora.com>:
> On Wed, 2017-07-05 at 08:46 -0600, Brian Paul wrote:
>> On 07/05/2017 12:57 AM, Robert Foss wrote:
>> > Add local strlcpy implementation.
>> >
>> > Signed-off-by: Robert Foss <robert.foss at collabora.com>
>> > ---
>> > Changes since v5:
>> >    Actually include changes from v5 in patch
>> >
>> > Changes since v4:
>> >    Gustaw Smolarczyk <wielkiegie at gmail.com>
>> >     - Make util_strlcpy have the same behaviour as strlcpy
>> >
>> > Changes since v3:
>> >    Matt Turner <mattst88 at gmail.com>
>> >     - Change name of util_strncpy to util_strlcpy
>> >
>> > Changes since v2:
>> >    Brian Paul <brianp at vmware.com>
>> >      - Patch added
>> >
>> >
>> >   src/util/u_string.h | 9 +++++++++
>> >   1 file changed, 9 insertions(+)
>> >
>> > diff --git a/src/util/u_string.h b/src/util/u_string.h
>> > index e88e13f42c..bbabcbc7cb 100644
>> > --- a/src/util/u_string.h
>> > +++ b/src/util/u_string.h
>> > @@ -48,6 +48,15 @@
>> >   extern "C" {
>> >   #endif
>> >
>> > +static inline size_t
>> > +util_strlcpy(char *dst, const char *src, size_t n)
>> > +{
>> > +   strncpy(dst, src, n);
>> > +   dst[n-1] = '\0';
>> > +
>> > +   return strnlen(src, n);
>> > +}
>>
>> This effectively walks over the source string twice.  I'd suggest
>> just
>> using your own loop.
>>
>
> I don't think a solution can be built using strlen if we want to only
> iterate once. So one iteration will have to do both counting and
> copying, which rules out using memcpy too.

I would say that iterating twice should be fine since we also get to
use the optimized strlen and memcpy routines. Your open-coded version
doesn't vectorize nor unroll on gcc-7 -Ofast (as far as my simple
testing showed).

That said, performance of this function doesn't matter much - what
matters the most is ease of use and correctness.

>
> So how about something like this:
>
> static inline size_t
> util_strlcpy(char *dst, const char *src, size_t n)
> {
>    size_t src_len = 0;
>
>    /* Copy chars for as long as there is space in dst */
>    while (src_len < (n - 1) && *src[src_len] != "\0") {

If n == 0, (n - 1) is the same as SIZE_MAX. I think you should check
for n == 0 and then just return strlen(src) before this loop is
entered. Or, alternatively, wrap the first loop and dst[src_len] =
'\0' assignment in if (n != 0) { ... }.
Also, use '\0' instead of "\0". And remove * before src[src_len].

>       dst[src_len] = src[src_len];
>       src_len++;
>    }
>
>    /* The last char of either src or if dst is filled
>     * should always be \0.
>     */
>    dst[src_len] = '\0';
>
>    /* Continue on finding the end of src */
>    while (src[src_len] != "\0") {

Use '\0' instead of "\0".

>       src_len++;
>    }
>
>    return src_len;
> }
>
> Rob.


Other than these, it looks correct.

Gustaw