Re: [Minios-devel] [UNIKRAFT PATCH] lib/nolibc: Add strtok to string.h

[Florian Schmidt <Florian.Schmidt@xxxxxxxxx>]

Hi Razvan,

thank you for the patch. Looks good, I just have a few small comments 
(see inline).
Cheers,
Florian

On 08/02/2018 06:05 PM, Razvan Cojocaru wrote:
> Added strtok and dependent functions to string.h
> These are required for the Xen network netfront driver.
>
> Functions are copied without modifications
> Taken from musl v1.1.19
> Commit <55df09bfccbfe21fc9dd7d8f94550c0ff25ace04>
>
> Signed-off-by: Razvan Cojocaru <razvan.cojocaru93@xxxxxxxxx>
> ---
>   lib/nolibc/include/string.h |  4 +++
>   lib/nolibc/string.c         | 74 +++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 78 insertions(+)
>
> diff --git a/lib/nolibc/include/string.h b/lib/nolibc/include/string.h
> index 4d12a5a..19fc5b1 100644
> --- a/lib/nolibc/include/string.h
> +++ b/lib/nolibc/include/string.h
> @@ -57,6 +57,10 @@ const char *strchr(const char *str, int c);
>   int strncmp(const char *str1, const char *str2, size_t len);
>   int strcmp(const char *str1, const char *str2);
>   
> +size_t strcspn(const char *s, const char *c);
> +size_t strspn(const char *s, const char *c);
> +char *strtok(char *restrict s, const char *restrict sep);
> +
>   #ifdef __cplusplus
>   }
>   #endif
> diff --git a/lib/nolibc/string.c b/lib/nolibc/string.c
> index bf89106..77b6798 100644
> --- a/lib/nolibc/string.c
> +++ b/lib/nolibc/string.c
> @@ -166,3 +166,77 @@ int strcmp(const char *str1, const char *str2)
>   
>   	return __res;
>   }
> +
> +/* Taken from musl libc */
> +#define ALIGN (sizeof(size_t))
> +#define ONES ((size_t) -1 / UCHAR_MAX)
> +#define HIGHS (ONES * (UCHAR_MAX / 2 + 1))
> +#define HASZERO(x) ((x) - ONES & ~(x) & HIGHS)
gcc throws parentheses warnings here, so I would rephrase this as
#define HASZERO(x) (((x) - ONES) & ~(x) & HIGHS)


> +#define BITOP(a, b, op) \
> +               ((a)[(size_t)(b) / (8*sizeof *(a))] op \
> +               (size_t)1 << ((size_t)(b) % (8 * sizeof *(a))))
> +
> +char *__strchrnul(const char *s, int c)
Since this function is a standard library function (though a GNU 
extension), and does exactly what is expected of it, I think we might as 
well name it strchrnul and add its declaration to string.h. musl 
provides the function with a weak_alias (though I'm not sure why they do 
it for only 8 out of over 100 string functions), so we can either do it 
that way, or just rename this to strchrnul.
So, my suggestion:
1) Add strchrnul declaration to string.h
2) Either rename __strchrnul in string.c to strchrnul, or add a 
weak_alias(__strchrnul, strchrnul)
Also, but this comes down to personal preference... since strchr is a 
two-line wrapper around strchrnul, we could also provide that. But 
that's a "convenience vs. minimality" argument, and I don't care much 
either way.
> +{
> +       size_t *w, k;
> +
> +       c = (unsigned char)c;
> +       if (!c)
> +               return (char *)s + strlen(s);
> +
> +       for (; (uintptr_t)s % ALIGN; s++)
> +               if (!*s || *(unsigned char *)s == c)
> +                       return (char *)s;
> +       k = ONES * c;
> +       for (w = (void *)s; !HASZERO(*w) && !HASZERO(*w^k); w++);
Here and in a few other places, checkpatch complains about "trailing 
statements should be on next line", and I tend to agree:
By formatting it like this,

for (w = (void *)s; !HASZERO(*w) && !HASZERO(*w ^ k); w++);
        ;

it's IMHO clearer that the body is supposed to be empty, because all 
work is done in the condition. (I would also add spaces around the ^ 
operator).

> +       for (s = (void *)w; *s && *(unsigned char *)s != c; s++);
> +       return (char *)s;
> +}
> +
> +size_t strcspn(const char *s, const char *c)
> +{
> +       const char *a = s;
> +       size_t byteset[32 / sizeof(size_t)];
> +
> +       if (!c[0] || !c[1])
> +               return __strchrnul(s, *c)-a;
> +
> +       memset(byteset, 0, sizeof(byteset));
> +       for (; *c && BITOP(byteset, *(unsigned char *)c, |=); c++);
> +       for (; *s && !BITOP(byteset, *(unsigned char *)s, &); s++);
> +       return s-a;
> +}
> +
> +size_t strspn(const char *s, const char *c)
> +{
> +       const char *a = s;
> +       size_t byteset[32 / sizeof(size_t)] = { 0 };
> +
> +       if (!c[0])
> +               return 0;
> +       if (!c[1]) {
> +               for (; *s == *c; s++);
> +               return s-a;
> +       }
> +
> +       for (; *c && BITOP(byteset, *(unsigned char *)c, |=); c++);
> +       for (; *s && BITOP(byteset, *(unsigned char *)s, &); s++);
> +       return s-a;
> +}
> +
> +char *strtok(char *restrict s, const char *restrict sep)
> +{
> +       static char *p;
> +
> +       if (!s && !(s = p))
> +               return NULL;
> +       s += strspn(s, sep);
> +       if (!*s)
> +               return p = 0;
> +       p = s + strcspn(s, sep);
> +       if (*p)
> +               *p++ = 0;
> +       else
> +               p = 0;
> +       return s;
> +}
--
Dr. Florian Schmidt
フローリアン・シュミット
Research Scientist,
Systems and Machine Learning Group
NEC Laboratories Europe
Kurfürsten-Anlage 36, D-69115 Heidelberg
Tel.     +49 (0)6221 4342-265
Fax:     +49 (0)6221 4342-155
e-mail:  florian.schmidt@xxxxxxxxx
============================================================
Registered at Amtsgericht Mannheim, Germany, HRB728558

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel