-
Notifications
You must be signed in to change notification settings - Fork 341
Code formatting #1336
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
base: main
Are you sure you want to change the base?
Code formatting #1336
Conversation
.clang-format
Outdated
| BreakConstructorInitializers: BeforeComma | ||
| BreakAfterJavaFieldAnnotations: false | ||
| BreakStringLiterals: false | ||
| ColumnLimit: 80 |
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.
Looking through the diff. There are lots of excessive line breaks!
I think we should go for al least 100.
https://lkml.org/lkml/2020/5/29/1317
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 will change to 100 and see how it looks. If there aren't any outstanding issues that would require a wider limit, we could settle for 100.
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.
At one time - I made a loop, that incremented by 5's and then saved off where the min diff was - I don't remember what the results were - but it would be easy to do it again...
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 tested with a similar loop and noticed that the larger the ColumnLimit, the fewer changes are generated. But the decrease in changes isn’t linear. The biggest impact is switching from 80 to 85, then the next big impact is from 85-90 and so on. Looking at the graph, it looks like the "elbow" is around value 100, after which the curve flattens a bit.
Testing ColumnLimit values from 80 to 140 (step 5) in .clang-format...
This script will modify .clang-format, run format.sh, count changes, and revert them.
Starting tests...
Testing ColumnLimit: 80
Updated ColumnLimit to: 80
Running format.sh...
Changes detected - Files: 103, Lines: 13583
Reverting changes...
Testing ColumnLimit: 85
Updated ColumnLimit to: 85
Running format.sh...
Changes detected - Files: 103, Lines: 12988
Reverting changes...
Testing ColumnLimit: 90
Updated ColumnLimit to: 90
Running format.sh...
Changes detected - Files: 103, Lines: 12588
Reverting changes...
Testing ColumnLimit: 95
Updated ColumnLimit to: 95
Running format.sh...
Changes detected - Files: 103, Lines: 12450
Reverting changes...
Testing ColumnLimit: 100
Updated ColumnLimit to: 100
Running format.sh...
Changes detected - Files: 102, Lines: 12337
Reverting changes...
Testing ColumnLimit: 105
Updated ColumnLimit to: 105
Running format.sh...
Changes detected - Files: 103, Lines: 12223
Reverting changes...
Testing ColumnLimit: 110
Updated ColumnLimit to: 110
Running format.sh...
Changes detected - Files: 103, Lines: 12149
Reverting changes...
Testing ColumnLimit: 115
Updated ColumnLimit to: 115
Running format.sh...
Changes detected - Files: 103, Lines: 12119
Reverting changes...
Testing ColumnLimit: 120
Updated ColumnLimit to: 120
Running format.sh...
Changes detected - Files: 103, Lines: 12044
Reverting changes...
Testing ColumnLimit: 125
Updated ColumnLimit to: 125
Running format.sh...
Changes detected - Files: 103, Lines: 11990
Reverting changes...
Testing ColumnLimit: 130
Updated ColumnLimit to: 130
Running format.sh...
Changes detected - Files: 103, Lines: 11947
Reverting changes...
Testing ColumnLimit: 135
Updated ColumnLimit to: 135
Running format.sh...
Changes detected - Files: 103, Lines: 11914
Reverting changes...
Testing ColumnLimit: 140
Updated ColumnLimit to: 140
Running format.sh...
Changes detected - Files: 103, Lines: 11909
Reverting changes...
Restoring original ColumnLimit: 100
Updated ColumnLimit to: 100
=================================================================================
RESULTS SUMMARY
=================================================================================
ColumnLimit | Modified Files | Total Line Changes
------------ | -------------- | ------------------
80 | 103 | 13583
85 | 103 | 12988
90 | 103 | 12588
95 | 103 | 12450
100 | 102 | 12337
105 | 103 | 12223
110 | 103 | 12149
115 | 103 | 12119
120 | 103 | 12044
125 | 103 | 11990
130 | 103 | 11947
135 | 103 | 11914
140 | 103 | 11909
=================================================================================
Minimum changes found with ColumnLimit: 140 (11909 line changes)
per discussion in #397, The clang-format is the linux kernel code formatting template with a few minor tweaks: indent continuation more: -ContinuationIndentWidth: 8 +ContinuationIndentWidth: 16 remove the kernel's ForEachMacros Sort the include paths, like Lars suggested in #411 -SortIncludes: false +SortIncludes: true In this order: -#IncludeBlocks: Preserve # Unknown to clang-format-5.0 +IncludeBlocks: Regroup IncludeCategories: - - Regex: '.*' - Priority: 1 -IncludeIsMainRegex: '(Test)?$' + - Regex: '^<.*\.h>' + Priority: 1 + - Regex: '^<.*' + Priority: 2 + - Regex: '.*' + Priority: 3 +IncludeIsMainRegex: 'xxx' The cmake-format.py file is from the doc, with tabs set to 4 spaces https://cmake-format.readthedocs.io/en/latest/configuration.html This is something for discussion, This is expected to have clang-format version 7.0.1-8 (tags/RELEASE_701/final) or higher. Signed-off-by: Robin Getz <robin.getz@analog.com>
Signed-off-by: Robin Getz <robin.getz@analog.com>
The script supports ignoring specific files and directories by reading their paths from a list, ensuring that any files or folders specified will be excluded from formatting. Signed-off-by: Dan Nechita <dan.nechita@analog.com>
On Windows, the include order matters so turn off clang-format to preserve the include order. Otherwise the windows build will fail. Signed-off-by: Dan Nechita <dan.nechita@analog.com>
Format all .c and .h files using rules from .clang-format but ignore paths defined in .clangformatignore file. Format all .cmake and CMakeLists.txt files using rules from cmake-format.py but ignore paths defined in .cmakeformatignore file. The current changes result from executing: ./format.sh in the repository's top-level directory. Signed-off-by: Dan Nechita <dan.nechita@analog.com>
c3de091 to
ad79729
Compare
|
V1:
|
|
Eventually (after this is merged), you will want to add something like this also here |
|
After fixing things - a pre-commit hook/check is needed to ensure that all future patches keep the style - or it will just start diverting... |
I have an active PR (#1338) that implements a CI job that checks if code is formatted. The job will fail if it's not. Specifically, the job checks all changed files, applies formatting, and fails if even one is not properly formatted. |
PR Description
This PR continues the effort to introduce code formatting rules, initially discussed in #397 and further developed in #413 by @rgetz.
Additionally, this PR provides a script that enables users to format the entire codebase.
PR Type
PR Checklist