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

[Jan Beulich <jbeulich@xxxxxxxx>]

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:
>>> @@ -43,12 +43,12 @@
>>>   
>>>   static struct ns16550 {
>>>       int baud, clock_hz, data_bits, parity, stop_bits, fifo_size, irq;
>>> -    u64 io_base;   /* I/O port or memory-mapped I/O address. */
>>> +    u64 io_base; /* I/O port or memory-mapped I/O address. */
>>>       u64 io_size;
>>>       int reg_shift; /* Bits to shift register offset by */
>> Breaking alignment between comments (also in the immediately following hunk),
>> while at the same time ...
> This one...
>>>       int reg_width; /* Size of access to use, the registers
>>>                       * themselves are still bytes */
>> ... not taking care of the comment style violation here?
> This is controlled by ReflowComments option [3]:
> 
>  From the tool point of view the comment is formatted correctly
> I didn't find a way to convert such comments into
> /*
>   * Size of access to use, the registers * themselves are still bytes */ if 
> this is what you mean.

Above you see what I received. I can't really deduce from this what
formatting you suggested. In the case at hand, though, I think it's not
the best idea anyway to put a multi-line comment past a declaration (or
statement).

       /*
        * Size of access to use, the registers
        * themselves are still bytes
        */
       int reg_width;

is what I think would be better in such a case (artificially keeping
this to be multi-line, even if it looks as if it might fit on just one
line then).

>>> @@ -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;

or

    return ((ns_read_reg(uart, UART_LSR) & uart->lsr_mask) == uart->lsr_mask)
           ? uart->fifo_size : 0;

or

    return ((ns_read_reg(uart, UART_LSR) & uart->lsr_mask) == uart->lsr_mask
            ? uart->fifo_size
            : 0);

(not going to list more variants which are all okay).

In any event, a fundamental requirement of mine is that such a tool would
only apply adjustments when and where style is actively violated. I.e. in
the case here:

   return ((ns_read_reg(uart, UART_LSR) &
            uart->lsr_mask) == uart->lsr_mask) ? uart->fifo_size : 0;

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.

>>> @@ -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. 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, right
now. And that in multiple projects all with different styles. I expect to be
in the position to do so also going forward. This, imo, requires that there
be left some room for variations. Which in turn requires that the tool would
leave alone anything that is not in conflict with the written down or defacto
style.

>>> @@ -335,14 +338,14 @@ static void ns16550_setup_preirq(struct ns16550 *uart)
>>>       else
>>>       {
>>>           /* Baud rate already set: read it out from the divisor latch. */
>>> -        divisor  = ns_read_reg(uart, UART_DLL);
>>> +        divisor = ns_read_reg(uart, UART_DLL);
>>>           divisor |= ns_read_reg(uart, UART_DLM) << 8;
>> An example where tabulation is being broken. There are quite a bit worse
>> ones further down.
> This one will be impossible to implement with clang-format IMO.
> Because there are cases where you want this (like above) and you know why:
> the assignments look prettier here that way. But this doesn't mean
> that in some other places other assignments will look got for you if
> formatted the same way.
> The question here what is the metric (human perception?) in this case
> and how this perception can be be programmed into clang-format
> configuration?

Which imo is a pretty strong argument against using any auto-formatting. At
least up to the point where AI would end up being smart enough to mimic this
human perception.

>>> @@ -350,8 +353,10 @@ static void ns16550_setup_preirq(struct ns16550 *uart)
>>>       ns_write_reg(uart, UART_MCR, UART_MCR_DTR | UART_MCR_RTS);
>>>   
>>>       /* Enable and clear the FIFOs. Set a large trigger threshold. */
>>> -    ns_write_reg(uart, UART_FCR,
>>> -                 UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX | 
>>> UART_FCR_TRG14);
>>> +    ns_write_reg(uart,
>>> +                 UART_FCR,
>>> +                 UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX |
>>> +                     UART_FCR_TRG14);
>> What's the underlying indentation rule here? The way it's re-formatted
>> certainly looks odd to me. What we occasionally do in such cases is add
>> parentheses:
>>
>>      ns_write_reg(uart,
>>                   UART_FCR,
>>                   (UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX |
>>                    UART_FCR_TRG14));
>>
>> Also - does the format they apply demand one argument per line? Else
>> why not
>>
>>      ns_write_reg(uart, UART_FCR,
>>                   (UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX |
>>                    UART_FCR_TRG14));
>>
>> Plus what's their criteria to choose between this style of function calls
>> and the one in context higher up for calls to pci_conf_write16()?
> This is controlled with AlignAfterOpenBracket [7]
> So this could be:
> *AlignAfterOpenBracket: Align*
> 
> -    ns_write_reg(uart, UART_FCR,
> -                 UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX | 
> UART_FCR_TRG14);
> +    ns_write_reg(uart,
> +                 UART_FCR,
> +                 UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX |
> +                     UART_FCR_TRG14);

As before indentation is bogus here, ...

> *AlignAfterOpenBracket: |DontAlign*
> 
> -    ns_write_reg(uart, UART_FCR,
> -                 UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX | 
> UART_FCR_TRG14);
> +    ns_write_reg(uart,
> +        UART_FCR,
> +        UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX | UART_FCR_TRG14);

... and even more so here, ...

> |*AlignAfterOpenBracket: |AlwaysBreak*
> 
> -    ns_write_reg(uart, UART_FCR,
> -                 UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX | 
> UART_FCR_TRG14);
> +    ns_write_reg(
> +        uart,
> +        UART_FCR,
> +        UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX | UART_FCR_TRG14);

... while this at least matches one of the forms we might presently use.
Yet again - there was nothing wrong with the original layout.

> |*AlignAfterOpenBracket: |BlockIndent*
> 
> -    ns_write_reg(uart, UART_FCR,
> -                 UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX | 
> UART_FCR_TRG14);
> +    ns_write_reg(
> +        uart,
> +        UART_FCR,
> +        UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX | UART_FCR_TRG14
> +    );

This clearly moves too far away from our present style.

>>> @@ -1706,7 +1704,7 @@ static void __init ns16550_parse_port_config(
>>>       if ( !parse_namevalue_pairs(str, uart) )
>>>           return;
>>>   
>>> - config_parsed:
>>> +config_parsed:
>> This is a no-go - ./CODING_STYLE specifically says why this isn't 
>> appropriate.
> Yes, it can't formatted as we wish. This is controlled with IndentGotoLabels 
> [10]
> and is a binary option, which leaves no means to disable it as both true and
> false will re-format the code
> 
> true:false:
> intf(){vs.intf(){
> if(foo()){if(foo()){
> label1:label1:
> bar();bar();
> }}
> label2:label2:
> return1;return1;
> }}

If there was some indentation meant to be in that blob, it was all lost,
I'm afraid. Still: The name of the control being IndentGotoLabels and it
being of boolean nature as you say, why would it do anything with an
already-indented label?

Jan