|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 1/2] code style: Format ns16550 driver
Hello, Jan! On 17.02.25 12:20, Jan Beulich wrote: On 16.02.2025 11:21, Oleksandr Andrushchenko wrote:--- a/xen/drivers/char/ns16550.c +++ b/xen/drivers/char/ns16550.c @@ -14,7 +14,7 @@ * abstracted. */ #if defined(CONFIG_HAS_PCI) && defined(CONFIG_X86) -# define NS16550_PCI +#define NS16550_PCI #endifHmm. Either form ought to be okay, so the line would want leaving untouched. This is controlled by PPIndentWidth option [1]: we have it set to "-1" which means "When set to -1 (default) *IndentWidth* is used also for preprocessor statements." Unfortunately, I was not able to see a change with PPIndentWidth + IndentWidth PPIndentWidth: -1 IndentWidth: 4 --- test_define.c 2025-02-19 11:20:30.708096428 +0200 +++ test_define_minus1.c 2025-02-19 11:21:42.313013373 +0200 @@ -1,5 +1,5 @@ #ifdef __linux__ -# define FOO +#define FOO #else -# define BAR +#define BAR #endif PPIndentWidth: 1 IndentWidth: 4 --- test_define.c 2025-02-19 11:20:30.708096428 +0200 +++ test_define_1.c 2025-02-19 11:23:46.986618706 +0200 @@ -1,5 +1,5 @@ #ifdef __linux__ -# define FOO +#define FOO #else -# define BAR +#define BAR #endif I thought it is a bug in the latest clang-format (21), but I was not able to get the expected result with clang-format 18 as well. I am not sure if I'm doing anything wrong with .clang-format, but this works one way for me as ow now. Takeaway: I was not able to control this, but the result seems to be acceptable It should be noted in the coding style though @@ -43,12 +43,12 @@static struct ns16550 { This one...
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.
... and this one. It seems that the tool tries to always have a single space before the comment. There is an option SpacesBeforeTrailingComments [2] which unfortunately only controls C++ style comments and "...does not affect trailing block comments (|/*| - comments)..." So, it seems that this is the rule to consider
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.
No, gain from human point of view But there is a gain that it is now formatted automatically.
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?
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); *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); |*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); |*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 + ); || | @@ -424,31 +430,37 @@ static void __init cf_check ns16550_init_postirq(struct serial_port *port)/* Calculate time to fill RX FIFO and/or empty TX FIFO for polling. */ Again, this is something which a human can decide what is better for their taste. clang-format won't be able to treat such cases differently @@ -1197,7 +1174,9 @@ pci_uart_config(struct ns16550 *uart, bool skip_amt, unsigned int idx)nextf = (f || (pci_conf_read16(PCI_SBDF(0, b, d, f), This is controlled with AlignArrayOfStructures [8] and we can have it as is if we set it to None The same was discussed before, so one can refresh that at [9]
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;
}}
All of the remarks aside, there are also a lot of good changes that are being made. Agree and at least applying some of those changes may improve the code to be more consistent. Jan Thank you, Oleksandr [1] https://clang.llvm.org/docs/ClangFormatStyleOptions.html#ppindentwidth [2] https://clang.llvm.org/docs/ClangFormatStyleOptions.html#spacesbeforetrailingcomments [3] https://clang.llvm.org/docs/ClangFormatStyleOptions.html#reflowcomments [4] https://clang.llvm.org/docs/ClangFormatStyleOptions.html#allowshortblocksonasingleline [5] https://clang.llvm.org/docs/ClangFormatStyleOptions.html#breakbeforeternaryoperators [6] https://clang.llvm.org/docs/ClangFormatStyleOptions.html#alignoperands [7] https://clang.llvm.org/docs/ClangFormatStyleOptions.html#alignafteropenbracket [8] https://clang.llvm.org/docs/ClangFormatStyleOptions.html#alignarrayofstructures [9] https://lists.xenproject.org/archives/html/xen-devel/2023-11/msg02172.html [10] https://clang.llvm.org/docs/ClangFormatStyleOptions.html#indentgotolabels
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |