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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 02/10/18 15:43, Jan Beulich wrote:
>>>> 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.

Yes, because that helper has the purpose of giving you one single argument.

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

It is plain grammar.  "count_arg" is only correct when the answer is 1.

>  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.)

Do you realise how hypocritical this statement is?  You frequently
insist on naming changes and hold up series because of it.  Perhaps the
best example recently is bfn/dfn, where bfn is a term which has been
used for 3 years at conferences and on xen-devel without objection so far.

You cannot expect reviews of your code to be held to a different
standard than you hold others to.

As for 3 months, I'm sorry that this series hasn't yet reached the top
of my priority list, but you, better than most, know exactly what has
been taking up all of our time during that period.  I'm getting through
my review backlog as fast as I can, but it doesn't preempt higher
priority tasks.  (As for today, review is happing while one of my slow
servers reboots...)

~Andrew

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