2017-07-06 20:52 GMT+02:00 Robert Foss <robert.f...@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.f...@collabora.com>
>> > ---
>> > Changes since v5:
>> > Actually include changes from v5 in patch
>> >
>> > Changes since v4:
>> > Gustaw Smolarczyk <wielkie...@gmail.com>
>> > - Make util_strlcpy have the same behaviour as strlcpy
>> >
>> > Changes since v3:
>> > Matt Turner <matts...@gmail.com>
>> > - Change name of util_strncpy to util_strlcpy
>> >
>> > Changes since v2:
>> > Brian Paul <bri...@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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
