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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

>> 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.
> 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.

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

>>   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.
> Not sure it is possible with any tool at all: it just makes the changes
> without distinguishing what can be skipped or not even if it does not
> violate the rules. It will always seek to improve or "improve" the
> code

Which by now I view as the core problem.

>>>>> @@ -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.
> Yes, sorry about that. The sample I was trying to put can be found at [2]

Funny, even with the setting "true" label2 there is unindented. We demand that
all labels be indented, even when - contextually - at function scope. (Nor do
we demand that labels be indented according to their - contextual - scope.)

Jan