Re: [Xen-devel] [PATCH v4 01/12] x86: infrastructure to allow converting certain indirect calls to direct ones

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 02.10.18 at 16:23, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 02/10/18 15:06, Jan Beulich wrote:
>>>>> On 02.10.18 at 15:21, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> On 02/10/18 11:12, Jan Beulich wrote:
>>>> --- a/xen/include/xen/lib.h
>>>> +++ b/xen/include/xen/lib.h
>>>> @@ -66,6 +66,10 @@
>>>>  
>>>>  #define ROUNDUP(x, a) (((x) + (a) - 1) & ~((a) - 1))
>>>>  
>>>> +#define count_va_arg_(dot, a1, a2, a3, a4, a5, a6, a7, a8, x, ...) x
>>>> +#define count_va_arg(args...) \
>>>> +    count_va_arg_(., ## args, 8, 7, 6, 5, 4, 3, 2, 1, 0)
>>> This particular bit of review split out for obvious reasons.
>>>
>>> We already have __count_args() in the ARM SMCCC infrastructure.  Please
>>> can we dedup that (broken out into a separate patch) rather than
>>> introducing a competing version.
>>>
>>> The ARM version is buggy.  It is off-by-two in the base case, and
>>> doesn't compile if fewer than two parameters are passed.
>> If you had followed earlier discussion, you'd have known up front
>> Julien's reaction. It is for that reason that I'm not trying to fiddle
>> with the ARM code in this regard, despite agreeing with you that
>> at the very least it _looks_ buggy.
>>
>>> This version functions correctly, but should be named with a plural.
>> Why plural? Nothing in stdarg.h uses plural (including the header
>> file name itself).
> 
> What has stdarg.h got to do with anything here?  (Irrespective, the name
> of the header file alone is the only thing which is grammatically
> questionable.)

And the identifier va_arg as well as the __builtin_va_arg it resolves
to are fine? It is precisely their naming that I've used to decide how
to name the macro here.

> count_va_args() should be plural for exactly the same reason that you
> named its parameter as 'args'.

That's your personal opinion. I very much think that the naming
here should not in any way block the series (and even less so when
the series has been out for review for almost 3 months, and through
a couple of iterations), as imo it is well within the bounds of what is
reasonable to let the submitter decide. (All of this is not to say that
I wouldn't make the change, if that's the only way to get the series
in, but it would be very reluctantly.)

The reason to name the arguments (in their entirety) "args" is, I
hope, quite obvious, and unrelated to the macro's name.

Jan



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