| Message ID | de68f8220fbb97ae6a3382138c23e81d0988a472.1700209834.git.federico.serafini@bugseng.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen: address some violations of MISRA C:2012 Rule 8.2 | expand |
On Fri, 17 Nov 2023, Federico Serafini wrote: > Add missing parameter names. No functional change. > > Signed-off-by: Federico Serafini <federico.serafini@bugseng.com> > --- > xen/include/xen/sort.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/xen/include/xen/sort.h b/xen/include/xen/sort.h > index 2f52ff85b9..1d5e3c5849 100644 > --- a/xen/include/xen/sort.h > +++ b/xen/include/xen/sort.h > @@ -23,8 +23,8 @@ > extern gnu_inline > #endif > void sort(void *base, size_t num, size_t size, > - int (*cmp)(const void *, const void *), > - void (*swap)(void *, void *, size_t)) > + int (*cmp)(const void *key, const void *elem), > + void (*swap)(void *a, void *b, size_t size)) > { > /* pre-scale counters for performance */ > size_t i = (num / 2) * size, n = num * size, c, r; Ideally we should also fix swap_memory_node, swap_mmio_handler
On 17.11.2023 09:40, Federico Serafini wrote: > --- a/xen/include/xen/sort.h > +++ b/xen/include/xen/sort.h > @@ -23,8 +23,8 @@ > extern gnu_inline > #endif > void sort(void *base, size_t num, size_t size, > - int (*cmp)(const void *, const void *), > - void (*swap)(void *, void *, size_t)) > + int (*cmp)(const void *key, const void *elem), Why "key" and "elem" here, but ... > + void (*swap)(void *a, void *b, size_t size)) ... "a" and "b" here? The first example of users of sort() that I'm looking at right now (x86/extable.c) is consistent in its naming. Jan
On 20/11/23 10:02, Jan Beulich wrote: > On 17.11.2023 09:40, Federico Serafini wrote: >> --- a/xen/include/xen/sort.h >> +++ b/xen/include/xen/sort.h >> @@ -23,8 +23,8 @@ >> extern gnu_inline >> #endif >> void sort(void *base, size_t num, size_t size, >> - int (*cmp)(const void *, const void *), >> - void (*swap)(void *, void *, size_t)) >> + int (*cmp)(const void *key, const void *elem), > > Why "key" and "elem" here, but ... > >> + void (*swap)(void *a, void *b, size_t size)) > > ... "a" and "b" here? The first example of users of sort() that I'm > looking at right now (x86/extable.c) is consistent in its naming. > On the Arm side there are {cmp,swap}_memory_node() and {cmp,swap}_mmio_handler(): "key"/"elem" are used for the comparison and "_a"/"_b" for the swap.
On 20.11.2023 14:13, Federico Serafini wrote: > On 20/11/23 10:02, Jan Beulich wrote: >> On 17.11.2023 09:40, Federico Serafini wrote: >>> --- a/xen/include/xen/sort.h >>> +++ b/xen/include/xen/sort.h >>> @@ -23,8 +23,8 @@ >>> extern gnu_inline >>> #endif >>> void sort(void *base, size_t num, size_t size, >>> - int (*cmp)(const void *, const void *), >>> - void (*swap)(void *, void *, size_t)) >>> + int (*cmp)(const void *key, const void *elem), >> >> Why "key" and "elem" here, but ... >> >>> + void (*swap)(void *a, void *b, size_t size)) >> >> ... "a" and "b" here? The first example of users of sort() that I'm >> looking at right now (x86/extable.c) is consistent in its naming. >> > > On the Arm side there are {cmp,swap}_memory_node() and > {cmp,swap}_mmio_handler(): "key"/"elem" are used for the comparison > and "_a"/"_b" for the swap. So - re-raising a question Stefano did raise - is Misra concerned about such discrepancies? If yes, _all_ instances need harmonizing. If not, I see no reason to go with misleading names here. Jan
On Mon, 20 Nov 2023, Jan Beulich wrote: > On 20.11.2023 14:13, Federico Serafini wrote: > > On 20/11/23 10:02, Jan Beulich wrote: > >> On 17.11.2023 09:40, Federico Serafini wrote: > >>> --- a/xen/include/xen/sort.h > >>> +++ b/xen/include/xen/sort.h > >>> @@ -23,8 +23,8 @@ > >>> extern gnu_inline > >>> #endif > >>> void sort(void *base, size_t num, size_t size, > >>> - int (*cmp)(const void *, const void *), > >>> - void (*swap)(void *, void *, size_t)) > >>> + int (*cmp)(const void *key, const void *elem), > >> > >> Why "key" and "elem" here, but ... > >> > >>> + void (*swap)(void *a, void *b, size_t size)) > >> > >> ... "a" and "b" here? The first example of users of sort() that I'm > >> looking at right now (x86/extable.c) is consistent in its naming. > >> > > > > On the Arm side there are {cmp,swap}_memory_node() and > > {cmp,swap}_mmio_handler(): "key"/"elem" are used for the comparison > > and "_a"/"_b" for the swap. > > So - re-raising a question Stefano did raise - is Misra concerned about > such discrepancies? If yes, _all_ instances need harmonizing. If not, I > see no reason to go with misleading names here. Federico confirmed that the answer is "no". I think we can use "key" and "elem" in this patch as they are more informative than "a" and "b"
On 21.11.2023 01:04, Stefano Stabellini wrote: > On Mon, 20 Nov 2023, Jan Beulich wrote: >> On 20.11.2023 14:13, Federico Serafini wrote: >>> On 20/11/23 10:02, Jan Beulich wrote: >>>> On 17.11.2023 09:40, Federico Serafini wrote: >>>>> --- a/xen/include/xen/sort.h >>>>> +++ b/xen/include/xen/sort.h >>>>> @@ -23,8 +23,8 @@ >>>>> extern gnu_inline >>>>> #endif >>>>> void sort(void *base, size_t num, size_t size, >>>>> - int (*cmp)(const void *, const void *), >>>>> - void (*swap)(void *, void *, size_t)) >>>>> + int (*cmp)(const void *key, const void *elem), >>>> >>>> Why "key" and "elem" here, but ... >>>> >>>>> + void (*swap)(void *a, void *b, size_t size)) >>>> >>>> ... "a" and "b" here? The first example of users of sort() that I'm >>>> looking at right now (x86/extable.c) is consistent in its naming. >>>> >>> >>> On the Arm side there are {cmp,swap}_memory_node() and >>> {cmp,swap}_mmio_handler(): "key"/"elem" are used for the comparison >>> and "_a"/"_b" for the swap. >> >> So - re-raising a question Stefano did raise - is Misra concerned about >> such discrepancies? If yes, _all_ instances need harmonizing. If not, I >> see no reason to go with misleading names here. > > Federico confirmed that the answer is "no". > > I think we can use "key" and "elem" in this patch as they are more > informative than "a" and "b" Except that "key" and "elem" are (imo) inapplicable to sort() callbacks (and inconsistent with the naming in the 2nd callback here); they _may_ be applicable in bsearch() ones. Note also how in the C99 spec these parameters of callback functions don't have names either. Jan
On Tue, 21 Nov 2023, Jan Beulich wrote: > On 21.11.2023 01:04, Stefano Stabellini wrote: > > On Mon, 20 Nov 2023, Jan Beulich wrote: > >> On 20.11.2023 14:13, Federico Serafini wrote: > >>> On 20/11/23 10:02, Jan Beulich wrote: > >>>> On 17.11.2023 09:40, Federico Serafini wrote: > >>>>> --- a/xen/include/xen/sort.h > >>>>> +++ b/xen/include/xen/sort.h > >>>>> @@ -23,8 +23,8 @@ > >>>>> extern gnu_inline > >>>>> #endif > >>>>> void sort(void *base, size_t num, size_t size, > >>>>> - int (*cmp)(const void *, const void *), > >>>>> - void (*swap)(void *, void *, size_t)) > >>>>> + int (*cmp)(const void *key, const void *elem), > >>>> > >>>> Why "key" and "elem" here, but ... > >>>> > >>>>> + void (*swap)(void *a, void *b, size_t size)) > >>>> > >>>> ... "a" and "b" here? The first example of users of sort() that I'm > >>>> looking at right now (x86/extable.c) is consistent in its naming. > >>>> > >>> > >>> On the Arm side there are {cmp,swap}_memory_node() and > >>> {cmp,swap}_mmio_handler(): "key"/"elem" are used for the comparison > >>> and "_a"/"_b" for the swap. > >> > >> So - re-raising a question Stefano did raise - is Misra concerned about > >> such discrepancies? If yes, _all_ instances need harmonizing. If not, I > >> see no reason to go with misleading names here. > > > > Federico confirmed that the answer is "no". > > > > I think we can use "key" and "elem" in this patch as they are more > > informative than "a" and "b" > > Except that "key" and "elem" are (imo) inapplicable to sort() callbacks > (and inconsistent with the naming in the 2nd callback here); they _may_ > be applicable in bsearch() ones. Note also how in the C99 spec these > parameters of callback functions don't have names either. Yes, reading the example in extable.c I think you are right. Maybe it is better to use "a" and "b" in both cmp and swap if you agree.
On 22.11.2023 02:19, Stefano Stabellini wrote: > On Tue, 21 Nov 2023, Jan Beulich wrote: >> On 21.11.2023 01:04, Stefano Stabellini wrote: >>> On Mon, 20 Nov 2023, Jan Beulich wrote: >>>> On 20.11.2023 14:13, Federico Serafini wrote: >>>>> On 20/11/23 10:02, Jan Beulich wrote: >>>>>> On 17.11.2023 09:40, Federico Serafini wrote: >>>>>>> --- a/xen/include/xen/sort.h >>>>>>> +++ b/xen/include/xen/sort.h >>>>>>> @@ -23,8 +23,8 @@ >>>>>>> extern gnu_inline >>>>>>> #endif >>>>>>> void sort(void *base, size_t num, size_t size, >>>>>>> - int (*cmp)(const void *, const void *), >>>>>>> - void (*swap)(void *, void *, size_t)) >>>>>>> + int (*cmp)(const void *key, const void *elem), >>>>>> >>>>>> Why "key" and "elem" here, but ... >>>>>> >>>>>>> + void (*swap)(void *a, void *b, size_t size)) >>>>>> >>>>>> ... "a" and "b" here? The first example of users of sort() that I'm >>>>>> looking at right now (x86/extable.c) is consistent in its naming. >>>>>> >>>>> >>>>> On the Arm side there are {cmp,swap}_memory_node() and >>>>> {cmp,swap}_mmio_handler(): "key"/"elem" are used for the comparison >>>>> and "_a"/"_b" for the swap. >>>> >>>> So - re-raising a question Stefano did raise - is Misra concerned about >>>> such discrepancies? If yes, _all_ instances need harmonizing. If not, I >>>> see no reason to go with misleading names here. >>> >>> Federico confirmed that the answer is "no". >>> >>> I think we can use "key" and "elem" in this patch as they are more >>> informative than "a" and "b" >> >> Except that "key" and "elem" are (imo) inapplicable to sort() callbacks >> (and inconsistent with the naming in the 2nd callback here); they _may_ >> be applicable in bsearch() ones. Note also how in the C99 spec these >> parameters of callback functions don't have names either. > > Yes, reading the example in extable.c I think you are right. Maybe it is > better to use "a" and "b" in both cmp and swap if you agree. Using a and b is (as it looks) in line with at least some uses we have, so less code churn than going with some other, more descriptive names (like left/right). So yes, I'm okay with using a/b. Jan
On 22/11/23 09:01, Jan Beulich wrote: > On 22.11.2023 02:19, Stefano Stabellini wrote: >> On Tue, 21 Nov 2023, Jan Beulich wrote: >>> On 21.11.2023 01:04, Stefano Stabellini wrote: >>>> On Mon, 20 Nov 2023, Jan Beulich wrote: >>>>> On 20.11.2023 14:13, Federico Serafini wrote: >>>>>> On 20/11/23 10:02, Jan Beulich wrote: >>>>>>> On 17.11.2023 09:40, Federico Serafini wrote: >>>>>>>> --- a/xen/include/xen/sort.h >>>>>>>> +++ b/xen/include/xen/sort.h >>>>>>>> @@ -23,8 +23,8 @@ >>>>>>>> extern gnu_inline >>>>>>>> #endif >>>>>>>> void sort(void *base, size_t num, size_t size, >>>>>>>> - int (*cmp)(const void *, const void *), >>>>>>>> - void (*swap)(void *, void *, size_t)) >>>>>>>> + int (*cmp)(const void *key, const void *elem), >>>>>>> >>>>>>> Why "key" and "elem" here, but ... >>>>>>> >>>>>>>> + void (*swap)(void *a, void *b, size_t size)) >>>>>>> >>>>>>> ... "a" and "b" here? The first example of users of sort() that I'm >>>>>>> looking at right now (x86/extable.c) is consistent in its naming. >>>>>>> >>>>>> >>>>>> On the Arm side there are {cmp,swap}_memory_node() and >>>>>> {cmp,swap}_mmio_handler(): "key"/"elem" are used for the comparison >>>>>> and "_a"/"_b" for the swap. >>>>> >>>>> So - re-raising a question Stefano did raise - is Misra concerned about >>>>> such discrepancies? If yes, _all_ instances need harmonizing. If not, I >>>>> see no reason to go with misleading names here. >>>> >>>> Federico confirmed that the answer is "no". >>>> >>>> I think we can use "key" and "elem" in this patch as they are more >>>> informative than "a" and "b" >>> >>> Except that "key" and "elem" are (imo) inapplicable to sort() callbacks >>> (and inconsistent with the naming in the 2nd callback here); they _may_ >>> be applicable in bsearch() ones. Note also how in the C99 spec these >>> parameters of callback functions don't have names either. >> >> Yes, reading the example in extable.c I think you are right. Maybe it is >> better to use "a" and "b" in both cmp and swap if you agree. > > Using a and b is (as it looks) in line with at least some uses we have, so > less code churn than going with some other, more descriptive names (like > left/right). So yes, I'm okay with using a/b. I'll send a new version.
diff --git a/xen/include/xen/sort.h b/xen/include/xen/sort.h index 2f52ff85b9..1d5e3c5849 100644 --- a/xen/include/xen/sort.h +++ b/xen/include/xen/sort.h @@ -23,8 +23,8 @@ extern gnu_inline #endif void sort(void *base, size_t num, size_t size, - int (*cmp)(const void *, const void *), - void (*swap)(void *, void *, size_t)) + int (*cmp)(const void *key, const void *elem), + void (*swap)(void *a, void *b, size_t size)) { /* pre-scale counters for performance */ size_t i = (num / 2) * size, n = num * size, c, r;
Add missing parameter names. No functional change. Signed-off-by: Federico Serafini <federico.serafini@bugseng.com> --- xen/include/xen/sort.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)