Re: [Xen-devel] [PATCH v8]xen: sched: convert RTDS from time to event driven model

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 12/03/2016 22:21, Chen, Tianyang wrote:
>
>
> On 03/11/2016 11:54 PM, Meng Xu wrote:
>> I'm focusing on the style and the logic in the replenish handler:
>>
>>>   /*
>>> @@ -160,6 +180,7 @@ struct rt_private {
>>>    */
>>>   struct rt_vcpu {
>>>       struct list_head q_elem;    /* on the runq/depletedq list */
>>> +    struct list_head replq_elem;/* on the repl event list */
>>
>> missing space before /*
>>
>
> I wasn't sure if all the comments should be lined up or not. Maybe
> there should be one more space for all the fields so they nicely line up?

At the very least, there should be a space between the ; and /

If you were introducing the entire structure at once then aligned
comments would be better, or if you were submitting a larger series with
some cleanup patches, then modifying the existing layout would be
acceptable.

In this case however, I would avoid aligning all the comments, as it
detracts from the clarity of the patch (whose purpose is a functional
change).

>
>>> +static int
>>> +deadline_queue_insert(struct rt_vcpu * (*_get_q_elem)(struct
>>> list_head *elem),
>>> +    struct rt_vcpu *svc, struct list_head *elem, struct list_head
>>> *queue)
>>> +{
>>> +    struct list_head *iter;
>>> +    int pos = 0;
>>> +
>>> +    list_for_each(iter, queue)
>>
>> space after ( and before )
>> If not sure about the space, we can refer to the sched_credit.c for
>> the style as well, beside the CODING_STYLE file. :-)
>>
>
> Ok. But in __runq_pick() there is no space. Also there is no space in
> the definition of this macro:
> #define list_for_each(pos, head) \
> Which one should I follow?
>

Apologies for that.  The code is not in a particularly good state, but
we do request that all new code adheres to the guidelines, in a hope
that eventually it will be consistent.

The macro definition won't have spaces because CPP syntax requires that
the ( be immediately following the macro name.  The Xen coding
guidelines require that control structures including if, for, while, and
these macro structures (being an extension of a for loop) have spaces
between the control name, and immediately inside the control brackets.

Therefore, it should end up as

...
list_for_each ( iter, queue )
{
...

If you wish, you are more than welcome to submit an additional patch
whose purpose is to specifically fix the pre-existing style issues, but
you are under no obligation to.

~Andrew

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