Re: [PATCH 1/2] code style: Format ns16550 driver

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.02.2025 16:40, Oleksandr Andrushchenko wrote:
> On 19.02.25 16:05, Jan Beulich wrote:
>> On 19.02.2025 14:52, Oleksandr Andrushchenko wrote:
>>> On 19.02.25 15:18, Jan Beulich wrote:
>>>> On 19.02.2025 13:39, Oleksandr Andrushchenko wrote:
>>>>> On 17.02.25 12:20, Jan Beulich wrote:
>>>>>> On 16.02.2025 11:21, Oleksandr Andrushchenko wrote:
>>>>>>> @@ -248,8 +249,9 @@ static int cf_check ns16550_tx_ready(struct 
>>>>>>> serial_port *port)
>>>>>>>         if ( ns16550_ioport_invalid(uart) )
>>>>>>>             return -EIO;
>>>>>>>     
>>>>>>> -    return ( (ns_read_reg(uart, UART_LSR) &
>>>>>>> -              uart->lsr_mask ) == uart->lsr_mask ) ? uart->fifo_size : 
>>>>>>> 0;
>>>>>>> +    return ((ns_read_reg(uart, UART_LSR) & uart->lsr_mask) == 
>>>>>>> uart->lsr_mask)
>>>>>>> +               ? uart->fifo_size
>>>>>>> +               : 0;
>>>>>> Indentation of the ? and : lines is clearly wrong here? What is the tool
>>>>>> doing?
>>>>> There are number of options that have influence on this formatting:
>>>>> AllowShortBlocksOnASingleLine [4]
>>>>> BreakBeforeTernaryOperators [5]
>>>>> AlignOperands [6]
>>>>>
>>>>> I was not able to tweak these options to have the previous form.
>>>> Right, sticking to the original form (with just the stray blanks zapped)
>>>> would of course be best. Yet again - the tool is doing more transformations
>>>> despite there not being any need. If, however, it does so, then one of my
>>>> expectations would be that the ? and : are properly indented:
>>>>
>>>>       return ((ns_read_reg(uart, UART_LSR) & uart->lsr_mask) == 
>>>> uart->lsr_mask)
>>>>              ? uart->fifo_size
>>>>              : 0;
>>> This only differs from what the tool is doing by the fact it applies
>>> the following rule: *IndentWidth: 4*, e.g. it has indented your construct
>>> by 4 spaces, see [1]. Which, IMO, is acceptable change.
>> I don't view this as acceptable. It falls in the same class then as
>>
>>      ns_write_reg(uart,
>>                   UART_FCR,
>>                   UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX |
>>                       UART_FCR_TRG14);
>>
>> that I also commented on in my initial reply.
> Ok, then how would you have it defined in the coding style as a rule?
> Such a diversity in defining indentation? So, will you have a dedicated
> rule for the ternary?

Well, this feels like you're returning a request I made your way, elsewhere.
Our present, unwritten rule for wrapping lines is to match the earlier
line's indentation (or the start of the expression), plus accounting for any
pending open parentheses, braces, or brackets. Hence why some consider this
form

     ns_write_reg(uart,
                  UART_FCR,
                  (UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX |
                   UART_FCR_TRG14));

preferable, as some tools (iirc e.g. Andrew indicated his editor does) then
are capable of inferring the intended indentation from the pending open
parentheses.

>>>> That's not overly neat wrapping, but in line with our style. If the other
>>>> form was demanded going forward, I'd be curious how you'd verbally
>>>> describe the requirement in ./CODING_STYLE.
>>> I believe this can be stated around the fact that we need to indent,
>>> e.g. apply the same rule as for other constructs already in use
>> Except here the tool didn't merely adjust indentation, but moved tokens
>> between lines.
> Again, if it moves, but doesn't break the style - then it is going to happen
> only once while applying big-scary-patch.

As to that patch: To some degree I actually like the idea of following Linux
in generally not allowing style-only patches.

>>>>>>> @@ -275,9 +277,10 @@ static void pci_serial_early_init(struct ns16550 
>>>>>>> *uart)
>>>>>>>     #ifdef NS16550_PCI
>>>>>>>         if ( uart->bar && uart->io_base >= 0x10000 )
>>>>>>>         {
>>>>>>> -        pci_conf_write16(PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1],
>>>>>>> -                                  uart->ps_bdf[2]),
>>>>>>> -                         PCI_COMMAND, PCI_COMMAND_MEMORY);
>>>>>>> +        pci_conf_write16(
>>>>>>> +            PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1], 
>>>>>>> uart->ps_bdf[2]),
>>>>>>> +            PCI_COMMAND,
>>>>>>> +            PCI_COMMAND_MEMORY);
>>>>>>>             return;
>>>>>>>         }
>>>>>> Hmm, transforming a well-formed block into another well-formed one. No
>>>>>> gain? (Same again further down.)
>>>>> No, gain from human point of view
>>>>> But there is a gain that it is now formatted automatically.
>>>> See above: I'd first like to see a written, textual description for all 
>>>> these
>>>> requirements. After all it needs to be possible for a human to write code
>>>> that the tool then wouldn't try to re-arrange. Which in turn requires that
>>>> the restrictions / constraints on the layout are spelled out.
>>> Agree, the existing coding style document will require some extension:
>>> at least clarifications and addition of the rules not described yet.
>>>>    I'm not looking
>>>> forward to pass all my patches through such a tool. I can write style-
>>>> conforming code pretty well, with - of course - occasional oversights,
>>> Which the tool will allow not to have for less accurate developers
>> I fear I don't understand this reply of yours.
> I mean that you can write such a well formatted code without any tool.
> But there are others who can't. Then the tool will help others to avoid
> code style violations.

And it'll screw me up (and possibly others too).

Jan