-
Notifications
You must be signed in to change notification settings - Fork 719
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
meta: add clang-format formatting configuration #1641
Conversation
This allows for voluntary formatting with clang-format (or IDE tools like clang-format plugin for VS Code). Once a suitable formatting configuration is found, automatic formatting can be configured Change-Id: I9021a98fe90c5ef9f21ce7459fdf05fa645fb56c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea.
I did not know clang-format tool.
BreakBeforeBinaryOperators: NonAssignment | ||
|
||
MaxEmptyLinesToKeep: 1 | ||
ColumnLimit: 140 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would vote for 100 maximum to fit two files on the reasonable-sized screen next to each other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Editors can wrap, but they hardly ever unwrap. Me for example like to glance at the structure of the file on a casual screen - thus as long lines as meaningful. There was also an option to preserve some of the breaks in the source as well (so that if clauses would maintain the breaks on || && operators for example)
In the end it would be good if there was a tool to run and not have any comments on PR-s regarding whitespace and indentation and related friction :)
I do feel that 80 and nearby numbers are IMHO historical lowest common denominators that could be forgotten - people have ultrawide screens (even though I also work on a 15" laptop or even smaller net terminal sometimes I would not define everything by these constraints)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Who would have thought that formatting is an endless spring of discussion?
Let's start with something objective. The recommended formatting should match what's already used throughout most of the code. Currently, the line length is way below 140 lines for most of the code. I personally, think that 80 characters are great to force programmers to write shorter functions (that still fit into one screen).
Now something productive. The recommended formatting needs to be automatically forced onto pull request. There are some people using it in CI already, e.g. https://github.com/r-lib/usethis/pull/22/files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pre-commit hook?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like this got a bit stalled from last year. I would really like to see the formatting a bit more formalized in both documentation and automation. I just ran simple test through the opensc repository to find that there is really many lines even longer than the 140 characters (mostly legacy code, tables/lists or generated cmdline).
$ grep '.\{140\}' `find -name "*.[c|h]"` | wc -l
315
$ grep '.\{120\}' `find -name "*.[c|h]"` | wc -l
703
$ grep '.\{110\}' `find -name "*.[c|h]"` | wc -l
1184
$ grep '.\{100\}' `find -name "*.[c|h]"` | wc -l
2225
$ grep '.\{80\}' `find -name "*.[c|h]"` | wc -l
8219
I think we all agree that we do not want to go as low as 80. I would like to see 100, but I can settle up with 120 as it overflows only few characters when displayed in two columns on my 24" screen. :)
Should Are you considering having a separate PR with only formatting changes? What version of clang-format are you proposing? I see there is a version 9. (Ubuntu-18.4 which I use has both 6 and 7.) What about: DerivePointerAlignment and PointerAlignment? |
BasedOnStyle: LLVM | ||
IndentWidth: 8 | ||
UseTab: ForIndentation | ||
BreakBeforeBraces: Attach |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the code is really Attach
, but I got used to Linux
here, which I find more readable (opening brace of function starts on new line) and I already used it some of the code already. This is something probably not for a change, but maybe for consideration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would be changing the AfterFunction
value before.
@@ -0,0 +1,28 @@ | |||
BasedOnStyle: LLVM | |||
IndentWidth: 8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also one thing that I am not sure if we are all on the same page. I use tab width 4 in my editor, but I am fine to adjust on 8 if we will be on the same page here.
For the automation, we should not depend only on pre-commit hooks as this would mean that the users would have to enable them (if I remember well). It should run also on CI and hopefully only on the changed lines in the PR. I do not like the idea of a large commit changing all the code to this strict formatting. With
I agree that we should use also One more question would go to the generated code (mostly I tried the following so far (I think we should really check only the diff of the changes in the PR, not the whole codebase in PRs):
Additionally, these formatting recommendation should be covered in contribution guide in human readable form. I think I can take care of that after we will settle down on some, |
I've left this open, because I didn't have the time to review all the possible settings. The formatting should be as close as possible to what most of the code uses. Thanks @Jakuje, for looking closely into this. I'm trusting your judgement on this. (I prefer using 8 spaces for a tab 😉) I have no idea how to resolve limiting the formatting only to the changes. The OpenSSL team solved this by creating one big commit that fixes all the formatting... Travis CI may offer more control over what to check. I haven't checked it into depth, but https://github.com/Sarcasm/run-clang-format looks like a suitable option, which also allows excluding stuff. Maybe integration of access tokens and visual output will become easier with Github Actions one day. |
Please, see the Jakuje#1 -- It already demonstrates how to use github actions to check only changed code (with some clever context matching) in the PR commits. It has advantage that it runs in matter of seconds after the push, rather than minutes as Travis because the actions are in container. It has also some failed attempts to do same thing in travis but last issue that I hit was the old clang so I would prefer going through the github actions. I will keep that branch for reference without fixing the issues yet. |
I would like OpenSC to consider using a feature of git 2.23 "--ignore-rev " to ignore formatting only commits when using git blame so one can see where the lines actually came from. See: git 2.23.0-RelNotes and a blog: ignoring-bulk-change-commits-with-git-blame We can take advantage of this today, so when developers start using git 2.23 OpenSC will already have a list of the formatting only commits. I would suggest, that we do one big commit, or groups of commits that are formatting only then have developers rebase on top, and start using the new format. Also see #1987 (comment) |
|
Thank you for the update on the The issue with too long lines allowed is that formatter will try to reflow the code if it was manually formatted to shorter line lengths for better readabilityeven if it is less readable. See for example https://gist.github.com/Jakuje/bd2317af7c4101f7e49c5d2686b414fd#file-cardos-5-3-patch-L550 |
(sorry for the close -- bad touchpad) |
It is not only "carefully crafted" long lines, but "carefully crafted" short line converted to hard to read long lines. I ran clang-client-7 on card-cardos.c in master. These are some of the changes it made:
And when considering a terminal size, consider the "Write" and "Preview" windows used to type this comment and the window you are reading this comment. They appear to be 110 characters per line. |
@Jakuje We are talking about the same hand crafted lines in card-cardos.c |
I think line break conditions can be tuned quite a bit... |
As I test to see what would be needed to do a mass conversion and how git blame 2.26 would work:
PROBLEM: Looking at other files, it looks like clang-format is reordering
|
Setting |
I think that all header files should be self containing. If scconf.h has some dependency to internal.h, it should include that file. The order should not matter in this case. (Sure, there are some exceptions to this principle, but here we have none of those) |
I've pushed 4e9cec1 for fixing the includes. |
The issue you describe in #1641 (comment) is caused by To avoid endless discussion about line lengths (and clang reformatting already formatted code), we can use Furthermore I updated the Another pain point is a lot of struct arrays aligned like this:
Clang format can not keep this format as described here. So before doing the mass-reformat, we should identify these array structs to skip formatting using Another common case for reformat is The updated
Some of them are helpful, some of them less, but I am not sure if we would be able to review this large changeset to filter out only the helpful parts. Having all of the formatting changes in one commit would be good, but we could try to do it iteratively per files or per changes, which will hopefully be more digestible, reviewable and in the end, we could end up with more commits in For changes going in, we should probably exclude the generated code in In the above PR it already suggest in comments fixes for the related code (would be good if it would go from some bot account). |
Is ".ignoreRevsFile" going to be added via a PR. Looks like blank lines and lines starting with "#" are comments. Do we plan on doing this before or after next release? I would suggest after, as there are a number of fixes that really need to be in next release ASAP, and we don't need to introduce any new bugs that might be caused by the formating. This will also then allow all developers to rebase on the next release, and any new commits can then check for the formating. This we would have only have one or a few .ignoreRevsFile and may not need any others. I added I can create a version for the current master. |
I am fine doing the whole reformat after next release. We still have many questions to answer about how to do that, how formatting will look like and how the CI checks should look like. For the formatting, I would like everyone who has at least some opinion about formatting, to have a look on my revisited
https://github.com/Jakuje/OpenSC/pull/1/files#diff-69a68f7ce1ea3687a38d8e4705e6fa48 |
I did a bit archeology and found the following page describing the coding style: https://github.com/OpenSC/OpenSC/wiki/Development-Policy Trying to follow Anyway, I did not get much feedback here nor in #2017 . The github actions look promissing, simple and fast. On the other hand, I like to see formatting suggestions as visible in Jakuje#1, but that would need separate "bot" account to do these comments. So unless there would be someone strictly against, I would merge changes to |
done with #2977 |
This allows for voluntary formatting with clang-format (or
IDE tools like clang-format plugin for VS Code).
Once a suitable formatting configuration is found, automatic formatting
can be configured
This formatting is used to format new files in esteid-2018 branch.
Change-Id: I9021a98fe90c5ef9f21ce7459fdf05fa645fb56c