[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/2] code style: Format ns16550 driver
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?
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.
Again, if it moves, but doesn't break the style - then it is going to happen
only once while applying big-scary-patch.
@@ -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.
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.
It depends. If this is the change made once, then I don't see it as
a lifetime 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.)
It seems that other projects (coding styles) see the alignment at function
scope differently. From Xen. If it was not implemented before in clang-format
could mean that either no other project use such an exception or there
are not so many projects use clang-format and didn't obviously face such
an issue
Jan
Thank you,
Oleksandr
|
