| Message ID | d28ea15aa8d36622548b433d5fb6f19dd521bdfb.1699369270.git.federico.serafini@bugseng.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [XEN,v3] xen/string: address violations of MISRA C:2012 Rules 8.2 and 8.3 | expand |
On Tue, 7 Nov 2023, Federico Serafini wrote: > Add missing parameter names and make function declarations and > definitions consistent. > Mismatches between parameter names "count" and "n" are resolved > in favor of "n", being the same name used by the C standard. > > No functional change. > > Signed-off-by: Federico Serafini <federico.serafini@bugseng.com> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
On 07.11.2023 16:18, Federico Serafini wrote: > Add missing parameter names and make function declarations and > definitions consistent. > Mismatches between parameter names "count" and "n" are resolved > in favor of "n", being the same name used by the C standard. I'm afraid this wasn't done consistently: > --- a/xen/include/xen/string.h > +++ b/xen/include/xen/string.h > @@ -12,27 +12,27 @@ > #define strncpy __xen_has_no_strncpy__ > #define strncat __xen_has_no_strncat__ > > -size_t strlcpy(char *, const char *, size_t); > -size_t strlcat(char *, const char *, size_t); > -int strcmp(const char *, const char *); > -int strncmp(const char *, const char *, size_t); > -int strcasecmp(const char *, const char *); > -int strncasecmp(const char *, const char *, size_t); > -char *strchr(const char *, int); > -char *strrchr(const char *, int); > -char *strstr(const char *, const char *); > -size_t strlen(const char *); > -size_t strnlen(const char *, size_t); > -char *strpbrk(const char *, const char *); > -char *strsep(char **, const char *); > -size_t strspn(const char *, const char *); > - > -void *memset(void *, int, size_t); > -void *memcpy(void *, const void *, size_t); > -void *memmove(void *, const void *, size_t); > -int memcmp(const void *, const void *, size_t); > -void *memchr(const void *, int, size_t); > -void *memchr_inv(const void *, int, size_t); > +size_t strlcpy(char *dest, const char *src, size_t size); > +size_t strlcat(char *dest, const char *src, size_t size); > +int strcmp(const char *cs, const char *ct); > +int strncmp(const char *cs, const char *ct, size_t count); There's still "count" here and ... > +int strcasecmp(const char *s1, const char *s2); > +int strncasecmp(const char *s1, const char *s2, size_t len); > +char *strchr(const char *s, int c); > +char *strrchr(const char *s, int c); > +char *strstr(const char *s1, const char *s2); > +size_t strlen(const char *s); > +size_t strnlen(const char *s, size_t count); > +char *strpbrk(const char *cs,const char *ct); > +char *strsep(char **s, const char *ct); > +size_t strspn(const char *s, const char *accept); > + > +void *memset(void *s, int c, size_t n); > +void *memcpy(void *dest, const void *src, size_t n); > +void *memmove(void *dest, const void *src, size_t n); > +int memcmp(const void *cs, const void *ct, size_t count); ... here (not counting functions which aren't part of the C standard). Otoh I'm unsure I understand that part of the description correctly: There was no disagreement for any of ... > --- a/xen/lib/memcpy.c > +++ b/xen/lib/memcpy.c > @@ -8,16 +8,16 @@ > * memcpy - Copy one area of memory to another > * @dest: Where to copy to > * @src: Where to copy from > - * @count: The size of the area. > + * @n: The size of the area. > * > * You should not use this function to access IO space, use memcpy_toio() > * or memcpy_fromio() instead. > */ > -void *(memcpy)(void *dest, const void *src, size_t count) > +void *(memcpy)(void *dest, const void *src, size_t n) > { > char *tmp = (char *) dest, *s = (char *) src; > > - while (count--) > + while (n--) > *tmp++ = *s++; > > return dest; > --- a/xen/lib/memmove.c > +++ b/xen/lib/memmove.c > @@ -8,23 +8,23 @@ > * memmove - Copy one area of memory to another > * @dest: Where to copy to > * @src: Where to copy from > - * @count: The size of the area. > + * @n: The size of the area. > * > * Unlike memcpy(), memmove() copes with overlapping areas. > */ > -void *(memmove)(void *dest, const void *src, size_t count) > +void *(memmove)(void *dest, const void *src, size_t n) > { > char *tmp, *s; > > if (dest <= src) { > tmp = (char *) dest; > s = (char *) src; > - while (count--) > + while (n--) > *tmp++ = *s++; > } else { > - tmp = (char *) dest + count; > - s = (char *) src + count; > - while (count--) > + tmp = (char *) dest + n; > + s = (char *) src + n; > + while (n--) > *--tmp = *--s; > } > > --- a/xen/lib/memset.c > +++ b/xen/lib/memset.c > @@ -8,15 +8,15 @@ > * memset - Fill a region of memory with the given value > * @s: Pointer to the start of the area. > * @c: The byte to fill the area with > - * @count: The size of the area. > + * @n: The size of the area. > * > * Do not use memset() to access IO space, use memset_io() instead. > */ > -void *(memset)(void *s, int c, size_t count) > +void *(memset)(void *s, int c, size_t n) > { > char *xs = (char *) s; > > - while (count--) > + while (n--) > *xs++ = c; > > return s; ... these, seeing that the declarations simply didn't have any parameter names at all. Jan
(re-adding xen-devel@) On 08.11.2023 09:43, Federico Serafini wrote: > On 08/11/23 09:07, Jan Beulich wrote: >> On 07.11.2023 16:18, Federico Serafini wrote: >>> Add missing parameter names and make function declarations and >>> definitions consistent. >>> Mismatches between parameter names "count" and "n" are resolved >>> in favor of "n", being the same name used by the C standard. >> >> I'm afraid this wasn't done consistently: >> >>> --- a/xen/include/xen/string.h >>> +++ b/xen/include/xen/string.h >>> @@ -12,27 +12,27 @@ >>> #define strncpy __xen_has_no_strncpy__ >>> #define strncat __xen_has_no_strncat__ >>> >>> -size_t strlcpy(char *, const char *, size_t); >>> -size_t strlcat(char *, const char *, size_t); >>> -int strcmp(const char *, const char *); >>> -int strncmp(const char *, const char *, size_t); >>> -int strcasecmp(const char *, const char *); >>> -int strncasecmp(const char *, const char *, size_t); >>> -char *strchr(const char *, int); >>> -char *strrchr(const char *, int); >>> -char *strstr(const char *, const char *); >>> -size_t strlen(const char *); >>> -size_t strnlen(const char *, size_t); >>> -char *strpbrk(const char *, const char *); >>> -char *strsep(char **, const char *); >>> -size_t strspn(const char *, const char *); >>> - >>> -void *memset(void *, int, size_t); >>> -void *memcpy(void *, const void *, size_t); >>> -void *memmove(void *, const void *, size_t); >>> -int memcmp(const void *, const void *, size_t); >>> -void *memchr(const void *, int, size_t); >>> -void *memchr_inv(const void *, int, size_t); >>> +size_t strlcpy(char *dest, const char *src, size_t size); >>> +size_t strlcat(char *dest, const char *src, size_t size); >>> +int strcmp(const char *cs, const char *ct); >>> +int strncmp(const char *cs, const char *ct, size_t count); >> >> There's still "count" here and ... >> >>> +int strcasecmp(const char *s1, const char *s2); >>> +int strncasecmp(const char *s1, const char *s2, size_t len); >>> +char *strchr(const char *s, int c); >>> +char *strrchr(const char *s, int c); >>> +char *strstr(const char *s1, const char *s2); >>> +size_t strlen(const char *s); >>> +size_t strnlen(const char *s, size_t count); >>> +char *strpbrk(const char *cs,const char *ct); >>> +char *strsep(char **s, const char *ct); >>> +size_t strspn(const char *s, const char *accept); >>> + >>> +void *memset(void *s, int c, size_t n); >>> +void *memcpy(void *dest, const void *src, size_t n); >>> +void *memmove(void *dest, const void *src, size_t n); >>> +int memcmp(const void *cs, const void *ct, size_t count); >> >> ... here (not counting functions which aren't part of the C standard). >> >> Otoh I'm unsure I understand that part of the description correctly: >> There was no disagreement for any of ... >> >>> --- a/xen/lib/memcpy.c >>> +++ b/xen/lib/memcpy.c >>> @@ -8,16 +8,16 @@ >>> * memcpy - Copy one area of memory to another >>> * @dest: Where to copy to >>> * @src: Where to copy from >>> - * @count: The size of the area. >>> + * @n: The size of the area. >>> * >>> * You should not use this function to access IO space, use memcpy_toio() >>> * or memcpy_fromio() instead. >>> */ >>> -void *(memcpy)(void *dest, const void *src, size_t count) >>> +void *(memcpy)(void *dest, const void *src, size_t n) >>> { >>> char *tmp = (char *) dest, *s = (char *) src; >>> >>> - while (count--) >>> + while (n--) >>> *tmp++ = *s++; >>> >>> return dest; >>> --- a/xen/lib/memmove.c >>> +++ b/xen/lib/memmove.c >>> @@ -8,23 +8,23 @@ >>> * memmove - Copy one area of memory to another >>> * @dest: Where to copy to >>> * @src: Where to copy from >>> - * @count: The size of the area. >>> + * @n: The size of the area. >>> * >>> * Unlike memcpy(), memmove() copes with overlapping areas. >>> */ >>> -void *(memmove)(void *dest, const void *src, size_t count) >>> +void *(memmove)(void *dest, const void *src, size_t n) >>> { >>> char *tmp, *s; >>> >>> if (dest <= src) { >>> tmp = (char *) dest; >>> s = (char *) src; >>> - while (count--) >>> + while (n--) >>> *tmp++ = *s++; >>> } else { >>> - tmp = (char *) dest + count; >>> - s = (char *) src + count; >>> - while (count--) >>> + tmp = (char *) dest + n; >>> + s = (char *) src + n; >>> + while (n--) >>> *--tmp = *--s; >>> } >>> >>> --- a/xen/lib/memset.c >>> +++ b/xen/lib/memset.c >>> @@ -8,15 +8,15 @@ >>> * memset - Fill a region of memory with the given value >>> * @s: Pointer to the start of the area. >>> * @c: The byte to fill the area with >>> - * @count: The size of the area. >>> + * @n: The size of the area. >>> * >>> * Do not use memset() to access IO space, use memset_io() instead. >>> */ >>> -void *(memset)(void *s, int c, size_t count) >>> +void *(memset)(void *s, int c, size_t n) >>> { >>> char *xs = (char *) s; >>> >>> - while (count--) >>> + while (n--) >>> *xs++ = c; >>> >>> return s; >> >> ... these, seeing that the declarations simply didn't have any parameter >> names at all. > > The disagreements are between parameter names used in > "xen/arch/string.c" and the ones used in "xen/lib/mem{cpy,move,set}.c". > > The reason why some of the "counts" are left is to reduce code churn: > I preferred to add the missing "count" on the declaration rather than > adding "n" to the declaration and also changing the name from "count" to > "n" in the definition. I'm not happy about the resulting inconsistency, but I can kind of accept the "reduce code churn" argument. But then still the description wants to say so (along with making clear where the disagreements were). These are all adjustments which aren't required for functionality, so it is even more relevant to explain properly why the code is touched despite technically all being fine. Jan
On 08/11/23 10:18, Jan Beulich wrote: > (re-adding xen-devel@) > > On 08.11.2023 09:43, Federico Serafini wrote: >> On 08/11/23 09:07, Jan Beulich wrote: >>> On 07.11.2023 16:18, Federico Serafini wrote: >>>> Add missing parameter names and make function declarations and >>>> definitions consistent. >>>> Mismatches between parameter names "count" and "n" are resolved >>>> in favor of "n", being the same name used by the C standard. >>> >>> I'm afraid this wasn't done consistently: >>> >>>> --- a/xen/include/xen/string.h >>>> +++ b/xen/include/xen/string.h >>>> @@ -12,27 +12,27 @@ >>>> #define strncpy __xen_has_no_strncpy__ >>>> #define strncat __xen_has_no_strncat__ >>>> >>>> -size_t strlcpy(char *, const char *, size_t); >>>> -size_t strlcat(char *, const char *, size_t); >>>> -int strcmp(const char *, const char *); >>>> -int strncmp(const char *, const char *, size_t); >>>> -int strcasecmp(const char *, const char *); >>>> -int strncasecmp(const char *, const char *, size_t); >>>> -char *strchr(const char *, int); >>>> -char *strrchr(const char *, int); >>>> -char *strstr(const char *, const char *); >>>> -size_t strlen(const char *); >>>> -size_t strnlen(const char *, size_t); >>>> -char *strpbrk(const char *, const char *); >>>> -char *strsep(char **, const char *); >>>> -size_t strspn(const char *, const char *); >>>> - >>>> -void *memset(void *, int, size_t); >>>> -void *memcpy(void *, const void *, size_t); >>>> -void *memmove(void *, const void *, size_t); >>>> -int memcmp(const void *, const void *, size_t); >>>> -void *memchr(const void *, int, size_t); >>>> -void *memchr_inv(const void *, int, size_t); >>>> +size_t strlcpy(char *dest, const char *src, size_t size); >>>> +size_t strlcat(char *dest, const char *src, size_t size); >>>> +int strcmp(const char *cs, const char *ct); >>>> +int strncmp(const char *cs, const char *ct, size_t count); >>> >>> There's still "count" here and ... >>> >>>> +int strcasecmp(const char *s1, const char *s2); >>>> +int strncasecmp(const char *s1, const char *s2, size_t len); >>>> +char *strchr(const char *s, int c); >>>> +char *strrchr(const char *s, int c); >>>> +char *strstr(const char *s1, const char *s2); >>>> +size_t strlen(const char *s); >>>> +size_t strnlen(const char *s, size_t count); >>>> +char *strpbrk(const char *cs,const char *ct); >>>> +char *strsep(char **s, const char *ct); >>>> +size_t strspn(const char *s, const char *accept); >>>> + >>>> +void *memset(void *s, int c, size_t n); >>>> +void *memcpy(void *dest, const void *src, size_t n); >>>> +void *memmove(void *dest, const void *src, size_t n); >>>> +int memcmp(const void *cs, const void *ct, size_t count); >>> >>> ... here (not counting functions which aren't part of the C standard). >>> >>> Otoh I'm unsure I understand that part of the description correctly: >>> There was no disagreement for any of ... >>> >>>> --- a/xen/lib/memcpy.c >>>> +++ b/xen/lib/memcpy.c >>>> @@ -8,16 +8,16 @@ >>>> * memcpy - Copy one area of memory to another >>>> * @dest: Where to copy to >>>> * @src: Where to copy from >>>> - * @count: The size of the area. >>>> + * @n: The size of the area. >>>> * >>>> * You should not use this function to access IO space, use memcpy_toio() >>>> * or memcpy_fromio() instead. >>>> */ >>>> -void *(memcpy)(void *dest, const void *src, size_t count) >>>> +void *(memcpy)(void *dest, const void *src, size_t n) >>>> { >>>> char *tmp = (char *) dest, *s = (char *) src; >>>> >>>> - while (count--) >>>> + while (n--) >>>> *tmp++ = *s++; >>>> >>>> return dest; >>>> --- a/xen/lib/memmove.c >>>> +++ b/xen/lib/memmove.c >>>> @@ -8,23 +8,23 @@ >>>> * memmove - Copy one area of memory to another >>>> * @dest: Where to copy to >>>> * @src: Where to copy from >>>> - * @count: The size of the area. >>>> + * @n: The size of the area. >>>> * >>>> * Unlike memcpy(), memmove() copes with overlapping areas. >>>> */ >>>> -void *(memmove)(void *dest, const void *src, size_t count) >>>> +void *(memmove)(void *dest, const void *src, size_t n) >>>> { >>>> char *tmp, *s; >>>> >>>> if (dest <= src) { >>>> tmp = (char *) dest; >>>> s = (char *) src; >>>> - while (count--) >>>> + while (n--) >>>> *tmp++ = *s++; >>>> } else { >>>> - tmp = (char *) dest + count; >>>> - s = (char *) src + count; >>>> - while (count--) >>>> + tmp = (char *) dest + n; >>>> + s = (char *) src + n; >>>> + while (n--) >>>> *--tmp = *--s; >>>> } >>>> >>>> --- a/xen/lib/memset.c >>>> +++ b/xen/lib/memset.c >>>> @@ -8,15 +8,15 @@ >>>> * memset - Fill a region of memory with the given value >>>> * @s: Pointer to the start of the area. >>>> * @c: The byte to fill the area with >>>> - * @count: The size of the area. >>>> + * @n: The size of the area. >>>> * >>>> * Do not use memset() to access IO space, use memset_io() instead. >>>> */ >>>> -void *(memset)(void *s, int c, size_t count) >>>> +void *(memset)(void *s, int c, size_t n) >>>> { >>>> char *xs = (char *) s; >>>> >>>> - while (count--) >>>> + while (n--) >>>> *xs++ = c; >>>> >>>> return s; >>> >>> ... these, seeing that the declarations simply didn't have any parameter >>> names at all. >> >> The disagreements are between parameter names used in >> "xen/arch/string.c" and the ones used in "xen/lib/mem{cpy,move,set}.c". >> >> The reason why some of the "counts" are left is to reduce code churn: >> I preferred to add the missing "count" on the declaration rather than >> adding "n" to the declaration and also changing the name from "count" to >> "n" in the definition. > > I'm not happy about the resulting inconsistency, but I can kind of accept > the "reduce code churn" argument. But then still the description wants to > say so (along with making clear where the disagreements were). These are > all adjustments which aren't required for functionality, so it is even > more relevant to explain properly why the code is touched despite > technically all being fine. Alright. Before sending another version I will wait a few hours to see if other maintainers have anything to say about "inconsistencies" vs "reduced code churn".
diff --git a/xen/include/xen/string.h b/xen/include/xen/string.h index b4d2217a96..bd4a8f48e9 100644 --- a/xen/include/xen/string.h +++ b/xen/include/xen/string.h @@ -12,27 +12,27 @@ #define strncpy __xen_has_no_strncpy__ #define strncat __xen_has_no_strncat__ -size_t strlcpy(char *, const char *, size_t); -size_t strlcat(char *, const char *, size_t); -int strcmp(const char *, const char *); -int strncmp(const char *, const char *, size_t); -int strcasecmp(const char *, const char *); -int strncasecmp(const char *, const char *, size_t); -char *strchr(const char *, int); -char *strrchr(const char *, int); -char *strstr(const char *, const char *); -size_t strlen(const char *); -size_t strnlen(const char *, size_t); -char *strpbrk(const char *, const char *); -char *strsep(char **, const char *); -size_t strspn(const char *, const char *); - -void *memset(void *, int, size_t); -void *memcpy(void *, const void *, size_t); -void *memmove(void *, const void *, size_t); -int memcmp(const void *, const void *, size_t); -void *memchr(const void *, int, size_t); -void *memchr_inv(const void *, int, size_t); +size_t strlcpy(char *dest, const char *src, size_t size); +size_t strlcat(char *dest, const char *src, size_t size); +int strcmp(const char *cs, const char *ct); +int strncmp(const char *cs, const char *ct, size_t count); +int strcasecmp(const char *s1, const char *s2); +int strncasecmp(const char *s1, const char *s2, size_t len); +char *strchr(const char *s, int c); +char *strrchr(const char *s, int c); +char *strstr(const char *s1, const char *s2); +size_t strlen(const char *s); +size_t strnlen(const char *s, size_t count); +char *strpbrk(const char *cs,const char *ct); +char *strsep(char **s, const char *ct); +size_t strspn(const char *s, const char *accept); + +void *memset(void *s, int c, size_t n); +void *memcpy(void *dest, const void *src, size_t n); +void *memmove(void *dest, const void *src, size_t n); +int memcmp(const void *cs, const void *ct, size_t count); +void *memchr(const void *s, int c, size_t n); +void *memchr_inv(const void *s, int c, size_t n); #include <asm/string.h> diff --git a/xen/lib/memcpy.c b/xen/lib/memcpy.c index afb322797d..5476121c0d 100644 --- a/xen/lib/memcpy.c +++ b/xen/lib/memcpy.c @@ -8,16 +8,16 @@ * memcpy - Copy one area of memory to another * @dest: Where to copy to * @src: Where to copy from - * @count: The size of the area. + * @n: The size of the area. * * You should not use this function to access IO space, use memcpy_toio() * or memcpy_fromio() instead. */ -void *(memcpy)(void *dest, const void *src, size_t count) +void *(memcpy)(void *dest, const void *src, size_t n) { char *tmp = (char *) dest, *s = (char *) src; - while (count--) + while (n--) *tmp++ = *s++; return dest; diff --git a/xen/lib/memmove.c b/xen/lib/memmove.c index 1ab79dfb28..99804352e6 100644 --- a/xen/lib/memmove.c +++ b/xen/lib/memmove.c @@ -8,23 +8,23 @@ * memmove - Copy one area of memory to another * @dest: Where to copy to * @src: Where to copy from - * @count: The size of the area. + * @n: The size of the area. * * Unlike memcpy(), memmove() copes with overlapping areas. */ -void *(memmove)(void *dest, const void *src, size_t count) +void *(memmove)(void *dest, const void *src, size_t n) { char *tmp, *s; if (dest <= src) { tmp = (char *) dest; s = (char *) src; - while (count--) + while (n--) *tmp++ = *s++; } else { - tmp = (char *) dest + count; - s = (char *) src + count; - while (count--) + tmp = (char *) dest + n; + s = (char *) src + n; + while (n--) *--tmp = *--s; } diff --git a/xen/lib/memset.c b/xen/lib/memset.c index e86afafd02..48a072cb51 100644 --- a/xen/lib/memset.c +++ b/xen/lib/memset.c @@ -8,15 +8,15 @@ * memset - Fill a region of memory with the given value * @s: Pointer to the start of the area. * @c: The byte to fill the area with - * @count: The size of the area. + * @n: The size of the area. * * Do not use memset() to access IO space, use memset_io() instead. */ -void *(memset)(void *s, int c, size_t count) +void *(memset)(void *s, int c, size_t n) { char *xs = (char *) s; - while (count--) + while (n--) *xs++ = c; return s;
Add missing parameter names and make function declarations and definitions consistent. Mismatches between parameter names "count" and "n" are resolved in favor of "n", being the same name used by the C standard. No functional change. Signed-off-by: Federico Serafini <federico.serafini@bugseng.com> --- Changes in v3: - applied changes discussed in the following thread https://lists.xenproject.org/archives/html/xen-devel/2023-08/msg00318.html Changes in v2: - memset() adjusted. --- xen/include/xen/string.h | 42 ++++++++++++++++++++-------------------- xen/lib/memcpy.c | 6 +++--- xen/lib/memmove.c | 12 ++++++------ xen/lib/memset.c | 6 +++--- 4 files changed, 33 insertions(+), 33 deletions(-)