[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/2] code style: Format ns16550 driver
Hello, Jan!
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:
@@ -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).
Sorry, for the formatting
/*
* Size of access to use, the registers
* themselves are still bytes
*/
This is what I was trying to send
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;
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.
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
@@ -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
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
@@ -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.
Well, you already have the answer: only human or AI? Obviously, this
cannot be done with any of the tools, IMO
@@ -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.
So, we can stick with this option setting: AlignAfterOpenBracket: AlwaysBreak
|*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.
Yes, sorry about that. The sample I was trying to put can be found at [2]
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?
Not really: unfortunately it is not an enum which would have "Leave" option
both true and false values *will* apply formatting, but different
Jan
Thank you,
Oleksandr
[1] https://clang.llvm.org/docs/ClangFormatStyleOptions.html#indentwidth
[2] https://clang.llvm.org/docs/ClangFormatStyleOptions.html#indentgotolabels
|
