[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 18:01, Jan Beulich wrote:
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.
I do understand that the tool needs to do the job and be able to fit
any coding style exists and not vice versa. But this is only in an
ideal world which doesn't exist yet: those tools are also developed by
an open source community and they also have some limited bandwidth.
I mean that bot Xen and some magic tool might need to co-exist and
accept each other. Or just decide not to use any.
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.
Well, yes. I can suggest that if we decide to provide a series of
style-only patches that we commit those with a fake authorship,
e.g. "Author: clang-format@xxxxxxxxxxxxxx"
@@ -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
Thank you,
Oleksandr
|
