Re: [Xen-devel] [PATCH 1/3] gnttab: Drop the frame parameter from acquire_grant_for_copy()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.08.17 at 12:50, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 25/08/17 11:13, Jan Beulich wrote:
>>>>> On 24.08.17 at 19:55, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> It is redundant with the *page parameter.  Rename the grant_frame parameter 
>>> to
>>> indicate that it is local, and highlight the correctness of the change.
>> I don't understand this second sentence: For one, grant_frame isn't
>> and wasn't a parameter (but a local variable), and then I also don't
>> see what is being highlighted to validate the correctness. Or am I
>> being mislead by it not reading "and to highlight ..."? So far I was
>> believing that such an omission is okay in German, but not in English.
> 
> Taking grant_frame out entirely at this point (is what I did originally,
> but) does not make the patch read as obviously-safe.  Changing its name
> causes the compiler to help spot all uses.

With the light ambiguity resulting from there having been a
function parameter of this very name.

>>> @@ -2238,10 +2240,9 @@ acquire_grant_for_copy(
>>>          active_entry_release(act);
>>>          grant_read_unlock(rgt);
>>>  
>>> -        rc = acquire_grant_for_copy(td, trans_gref, rd->domain_id,
>>> -                                    readonly, &grant_frame, page,
>>> -                                    &trans_page_off, &trans_length,
>>> -                                    false);
>>> +        rc = acquire_grant_for_copy(
>>> +            td, trans_gref, rd->domain_id, readonly, page, &trans_page_off,
>>> +            &trans_length, false);
>> As hinted at by Wei, I'd prefer if you didn't alter the style to the
>> uglier variant - we really should use that only if the commonly
>> used one really gets unwieldy, which imo is not the case here.
> 
> Ugly? What is ugly (in general) is squashing all parameters together on
> many lines on the RHS.

As always with style it's a matter of taste: To me, the indentation
of the style you switch to looks always odd, as does the line break
right after the opening parenthesis (and this would extend to
other expressions too). Of course, ./CODING_STYLE has nothing
on this at all - "Long lines should be split at sensible places and the
trailing portions indented" leaves open what "sensible" is, or how
deep indentation ought to be.

Anyway, I certainly won't block the patch over this, I just find it
odd for style to be changed here without real need.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel