Update clang-format style #4073
Conversation
[no changelog]
[no changelog]
|
Regarding the proposed changes:
In this regard, i would be ok with changing the style. However, my worries here are mainly side-effects: Given this, and with regard to the fact that most formatting is matter of personal preference or habits, i would be inclined to be conservative with this. |
|
I like the style and would also go for 120 columns, though the cost of diffing with history would be high. |
|
I don't touch the code these days too much, so I do not want to be an authority in this decision, but I think that changing the style is usually not a good idea because it makes diffing to old code hard. If one prefers to have another style, they can apply it locally using clang-format and then reapply the original one just before commit. Subjective takes on proposed points:
|
|
One of the reasons why we decided to adopt Google's code style without customizations was to avoid delving into code formatting discussions, which are a matter of personal taste. After all, the team of developers working with the code is always changing. I still think it was a good decision. That being said, code styling nuances such as those in points 1, 4 and 5 do not make any difference to me. As for the line limit, the only time when I would prefer a larger limit is to fit a comment at the end of the line. When I am working on a single monitor, I appreciate the narrow line limit, so that I can split the screen and still see all the code. This especially applies to side-by-side diffs. I think it should be possible to do a side-by-side diff in GitHub on a full HD monitor with standard font size settings without the browser introducing extra line breaks. Setting the limit to 100 would probably break this (untested), and I am not convinced that 80 vs. 100 makes any difference in the first place. Even though more and more laptop computers have screens larger than 1920px in width, their physical dimensions remain more or less the same, so one generally needs to increase the font size to accommodate, resulting in the same amount of text fitting on the screen. As for point 3 (modify aligning of list arguments), again in terms of readability it makes no difference to me. However, it could help to simplify diffs. In particular, changing the length of the function name, such as
There is no difference in the easiness of writing code. I never try to format the code myself. I just write it and run the formatter when I am done. I strongly share @TychoVrahe's concern about the side-effects:
Code style commits do waste time when investigating the Git history.
Not only open PRs, any work-in-progress branches. It also causes conflicts when reverting old commits.
I'd rather not open the tabs vs. spaces wars, but I like that spaces ensure consistent alignment. If someone has strong formatting preferences, then they should do what @prusnak proposed:
In conclusion I would rather maintain the current style in part because I think it's a waste of time to change the code style every time the team composition changes and preferences shift, but more importantly, because of the git history diffs/blame issue. IMHO there doesn't seem to be sufficient reason to apply changes proposed in points 1, 2, 4 or 5 or to change spaces to tabs. The only style changes that I would consider of use are the following:
|
|
I have to admit this PR is more about my personal preference that rational reasons. That's just the way it is:-) I hoped to find allies in this. If not, we can completely discard it without any problem. If you feel that this should be stopped, please let me know. Side-effect of loosing diffs Yes, it's very undesirable and the strongest argument against it, but it's inevitable if we want to "improve." I understand it's a big price to pay, and if the improvements aren't worth it, this could eventually become a blocker. Conflicts with op PRs Yes, that's a problem we'll have to tackle, but only for a limited time period.
Tabs vs spaces If believe that using tabs is not an option when an inherent part of the formatting style involves #define AAA // Comment
#define AAAA // Comment
#define AAAAA // CommentIncreasing line limit Indentation For me personally, the most importand style change is also #3. I would also prefer having each argument on its own line if they don't fit on a single line. However, this will require trickier clang-format settings, if it’s even possible. But I'll try to find it. Change #4 is just a supplement to #3 to avoid a code like this: void function(
int argument1, int argument2, int argument3) {
int variable1;
int variable2;
....
}Change #5 is a minor detail, but I included it because it's consistent with #4. |
|
Finally, I found the If we don't apply 1) and 2), the number and nature of the changes in this case are quite reasonable. Blame/diff will not be so problematic. However, the result appears somewhat untidy, particularly in the headers, where some functions are on a single line while others have their arguments split across multiple lines. ---
BasedOnStyle: Google
# Increase indentation from 2 to 4 characters
#IndentWidth: 4
#ContinuationIndentWidth: 4
# Increase column limit from 80 to 100 characters
#ColumnLimit: 100
# Sort each #include blocks separately
IncludeBlocks: Preserve
# Break before braces on function, namespace and class definition
BreakBeforeBraces: Linux
# Always break after an open bracket, if the parameters don’t fit on a single line
AlignAfterOpenBracket: AlwaysBreak
# Disable short functions on single line
AllowShortFunctionsOnASingleLine: None
# If false, a function declaration’s or function definition’s parameters will
# either all be on the same line or will have one line each.
BinPackParameters: false
# If false, a function call’s arguments will either be all on the
# same line or will have one line each.
BinPackArguments: false
# This prevents all parameters from being placed on the next line, unless necessary.
AllowAllParametersOfDeclarationOnNextLine: falseSee the result: uint32_t ui_screen_install_confirm(
const vendor_header *const vhdr,
const image_header *const hdr,
secbool should_keep_seed,
secbool is_newvendor,
int version_cmp)
{
uint8_t fingerprint[32];
char ver_str[64];
get_image_fingerprint(hdr, fingerprint);
format_ver("%d.%d.%d", hdr->version, ver_str, sizeof(ver_str));
return screen_install_confirm(
vhdr->vstr,
vhdr->vstr_len,
ver_str,
fingerprint,
should_keep_seed == sectrue,
is_newvendor == sectrue,
version_cmp);
}But there's still some alignment that I’m not very happy with (and probably can't be altered): if (sectrue != check_image_header_sig(
current_hdr,
current_vhdr->vsig_m,
current_vhdr->vsig_n,
current_vhdr->vpub)) {
*is_new = sectrue;
return;
} |
|
I just found out that you can put a list of reformatting commits into a file like |
|
Adding if (sectrue !=
check_image_header_sig(
current_hdr,
current_vhdr->vsig_m,
current_vhdr->vsig_n,
current_vhdr->vpub)) {
*is_new = sectrue;
return;
}Notice that the default Google style mixes indentation using 2 and 4 spaces. Its default setting are |
|
Just to add my two cents to the discussion:
I've just tried it myself, and it's not as straightforward as it may seem. |
|
heard you're 2c short so here are mine:
|
|
At this point, its clear that we are not reaching consensus about the proposed changes so we will keep status quo. However, we might still be able to reach consensus for smaller and more limited scope: 1.) Modify list of arguments so that each argument is on separate line
2.) Disabled short functions on single line (original point 5)
And we would introduce the @cepetr could you please update this PR so we can see the resulting changes and discuss whether this would be OK for all? No rush, whenever you feel like it. |
I am OK with both changes ("each argument is on separate line" and "disable short functions") |
|
@cepetr , If this pull request will be ever merged, could you also include the following change in the same commit? We decided to apply formatting to some files that were unformated but we want to keep the number of reformatting commits as low as possible (see #4107 (comment)). diff --git a/tools/style.c.exclude b/tools/style.c.exclude
index 5ef29b507..adeb5ca60 100644
--- a/tools/style.c.exclude
+++ b/tools/style.c.exclude
@@ -3,9 +3,7 @@
^\./crypto/chacha20poly1305/
^\./crypto/ed25519-donna/
^\./crypto/gui/
-^\./crypto/blake2
^\./crypto/check_mem
-^\./crypto/groestl
^\./crypto/ripemd160
^\./crypto/segwit_addr
^\./crypto/sha2 |
Unfortunatelly I couldn't find a clang-format configuration to consistently put each parameter on a separate line when the function arguments can fit on a single line. This leads to inconsistent formatting, with some functions having arguments on one line and others spread across multiple lines, which looks messy, especially in .h files. Clang-format just isn't as flexible as it needs to be for this. const void *flash_get_address(uint8_t sector, uint32_t offset, uint32_t size);
secbool __wur flash_erase_sectors(
const uint8_t *sectors,
int len,
void (*progress)(int pos, int len));
secbool __wur flash_write_byte(uint8_t sector, uint32_t offset, uint8_t data);
secbool __wur flash_write_word(uint8_t sector, uint32_t offset, uint32_t data); |
|
After discussing this matter with @TychoVrahe today, I’ve decided to close it. Unfortunately, it seems there isn’t a viable way to strike a balance between what we want, the value of the change, and the limitations of clang-format. I truly appreciate everyone’s contributions and insights on this topic. Thank you. |
This PR proposes updates to the clang-format style to address certain preferences that some of us have regarding the default Google style:
Finally I decided to open this topic, even though I realize it may not be very productive. I have proposed just a few changes that I believe can bring some improvements: Please let me know if you like it or not.
1) Increase indentation from 2 to 4 spaces
This change aims to improve code readability by making it easier to locate the closing bracket of a block.
2) Increase line limit from 80 to 100 columns
An 80-column limit can be restrictive and cause unnecessary line breaks. Modern displays are wide enough to accommodate a 100-column limit, making this old-school rule less relevant.
3) Modify aligning of list arguments
The original settings often result in a lot of code on the right side of the screen, leaving blank areas on the left and making the alignment heavily dependent on the length of function names. The new settings are more compact and natural, making the code easier to write and read.
4) Add break before open braces in function definition (Linux style)
This setting helps to visually separate the function definition from the function body.
5) Disabled short functions on single line