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 17:40, <andrew.cooper3@xxxxxxxxxx> wrote:
> 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.

I know I did bring up this ambiguity of the term "bus" long before.

Also note the (significant imo) difference between a singular / plural
controversy, and one over possible ambiguities (which may make
already hard to understand code even harder to understand).

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

And I don't, or at least I try not 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...)

This is all understood, but this particular series (and at least one other
one), at least large parts of it, has been reviewed by others, so you
_could_ (but of course you don't have to) rely on those other reviews.

I hope, however, that you also understand that this almost-no-progress
situation is frustrating here. For patches submitted by others, I can
stand in for you when you're buried in higher priority tasks, but this does
not work for my own patches.

Jan


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