Re: [PATCH v7 08/16] emul/ns16x50: implement MCR/MSR registers

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.09.2025 10:00, Mykola Kvach wrote:
> Hi Jan,
> 
> On Mon, Sep 15, 2025 at 5:49 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>
>> On 15.09.2025 08:00, Mykola Kvach wrote:
>>> On Tue, Sep 9, 2025 at 12:12 AM <dmukhin@xxxxxxx> wrote:
>>>> --- a/xen/common/emul/vuart/ns16x50.c
>>>> +++ b/xen/common/emul/vuart/ns16x50.c
>>>> @@ -107,7 +107,7 @@ static bool cf_check ns16x50_iir_check_thr(const 
>>>> struct vuart_ns16x50 *vdev)
>>>>
>>>>  static bool cf_check ns16x50_iir_check_msi(const struct vuart_ns16x50 
>>>> *vdev)
>>>>  {
>>>> -    return false;
>>>> +    return vdev->regs[UART_MSR] & UART_MSR_CHANGE;
>>>>  }
>>>>
>>>>  /*
>>>> @@ -232,12 +232,63 @@ static int ns16x50_io_write8(
>>>>              regs[UART_LCR] = val;
>>>>              break;
>>>>
>>>> +        case UART_MCR: {
>>>
>>> Probably the opening brace should be moved to the next line.
>>> See CODING_STYLE:
>>>
>>> Braces ('{' and '}') are usually placed on a line of their own, except
>>> for:
>>>
>>> - the do/while loop
>>> - the opening brace in definitions of enum, struct, and union
>>> - the opening brace in initializers
>>> - compound literals
>>
> 
> Thanks for clarifying.
> 
>> strictly by the wording of the doc you're right, yet if you go look then
>> you'll see that we really permit both forms (and apparently prefer the
>> one used here).
> 
> I just want to make sure I understand the expectation correctly.
> The CODING_STYLE document has wording about brace placement, but as
> you noted, the actual code in this subsystem uses both styles, and the
> one used here seems to be preferred in practice.
> 
> To get a better sense, I did a quick search in the repository. The
> pattern with the brace on the next line after case appears roughly
> 340 times, while the variant with the brace on the same line as case
> appears about 75 times. So overall the first form seems to be much
> more common.
> 
> That makes me think the choice here is more a matter of maintainer
> preference than a global convention. My main concern is consistency:
> if in one place both forms are accepted, but in another case reviewers
> point back to the document and ask for strict compliance, it could
> create confusion for contributors.
> 
> I'm fine if Denis leaves it as is. I just wanted to note the
> misalignment with the CODING_STYLE doc.

Yes, the situation with ./CODING_STYLE is suboptimal. Yet trying to get
in changes to that file also has proven difficult.

As to the brace placement in case block: Please realize that this is
also special because of the case labels indented as much as switch()
statement's opening figure brace. While nothing can be done for the
closing braces (i.e. there being successive ones with the same
indentation), the opening ones have this alternative placement as an
option.

What we could consider is to allow omitting the braces altogether in
case blocks. That comes with its own downsides, but we may want to
weigh things as to what's deemed worse.

Jan