Skip to content
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

Allow using clang-format for format the code #291

Merged
merged 7 commits into from
Sep 16, 2022

Conversation

andrea-iob
Copy link
Member

In order to keep the code style consistent among all bitpit modules, I've added to the root of the repository the configuration file needed by clang-format. The presence of this configuration file allows to use clang-format to automatically format the code:

clang-format --style=file -i <FILENAME>

The configuration filed is based on the LLVM coding standards (https://llvm.org/docs/CodingStandards.html). The changes with respect to the base configuration are minor: the width of the indentation and the placement of the braces. Here the difference between the base LLVM configuration:

@@ -29,27 +29,27 @@
 BinPackArguments: true
 BinPackParameters: true
 BraceWrapping:
-  AfterCaseLabel:  false
-  AfterClass:      false
+  AfterCaseLabel:  true
+  AfterClass:      true
   AfterControlStatement: Never
-  AfterEnum:       false
-  AfterFunction:   false
+  AfterEnum:       true
+  AfterFunction:   true
   AfterNamespace:  false
   AfterObjCDeclaration: false
-  AfterStruct:     false
-  AfterUnion:      false
+  AfterStruct:     true
+  AfterUnion:      true
   AfterExternBlock: false
-  BeforeCatch:     false
+  BeforeCatch:     true
   BeforeElse:      false
-  BeforeLambdaBody: false
-  BeforeWhile:     false
+  BeforeLambdaBody: true
+  BeforeWhile:     true
   IndentBraces:    false
   SplitEmptyFunction: true
   SplitEmptyRecord: true
   SplitEmptyNamespace: true
 BreakBeforeBinaryOperators: None
 BreakBeforeConceptDeclarations: true
-BreakBeforeBraces: Attach
+BreakBeforeBraces: Custom
 BreakBeforeInheritanceComma: false
 BreakInheritanceList: BeforeColon
 BreakBeforeTernaryOperators: true
@@ -57,7 +57,7 @@
 BreakConstructorInitializers: BeforeColon
 BreakAfterJavaFieldAnnotations: false
 BreakStringLiterals: true
-ColumnLimit:     80
+ColumnLimit:     0
 CommentPragmas:  '^ IWYU pragma:'
 QualifierAlignment: Leave
 CompactNamespaces: false
@@ -104,12 +104,12 @@
 IndentPPDirectives: None
 IndentExternBlock: AfterExternBlock
 IndentRequires:  false
-IndentWidth:     2
+IndentWidth:     4
 IndentWrappedFunctionNames: false
 InsertTrailingCommas: None
 JavaScriptQuotes: Leave
 JavaScriptWrapImports: true
-KeepEmptyLinesAtTheStartOfBlocks: true
+KeepEmptyLinesAtTheStartOfBlocks: false
 LambdaBodyIndentation: Signature
 MacroBlockBegin: ''
 MacroBlockEnd:   ''
@@ -179,7 +179,7 @@
 StatementMacros:
   - Q_UNUSED
   - QT_REQUIRE_VERSION
-TabWidth:        8
+TabWidth:        4
 UseCRLF:         false
 UseTab:          Never
 WhitespaceSensitiveMacros:

If needed, the configuration can be changed (with the current one I get a code style very similar to the one we are currently using in some modules).

I've also added some scripts that allow to setup a pre-commit git hook to automatically format the code. The hook will inspect the code to be commited and, if it doesn't respect the code style, it will allow to automatically fix the coding style. This script can also be called manually to format individual files or staged changes. More information about the script here. The pre-commit hook can only be installed locally (see previous link), github doesn't support hooks. To enforce coding style on github, we can setup a github action that performs the check and allow merging a pull request only if the check is satisfied.

If needed, I can run clang-format on the whole code base or we can choose to apply the enforce the coding style only to the newly added code.

@andrea-iob
Copy link
Member Author

If you want to test different clang-format configurations, you can try this configurator.

@andrea-iob
Copy link
Member Author

There is a plugin for Eclipse that integrates the clang-format tool as an alternative C/C++ code formatter.

@marcocisternino
Copy link
Member

Several VSCode extension to clang-format are available (xaver's one is the most downloaded)

@marcocisternino
Copy link
Member

We have a code style guide here.
@roccoarpa , I'm not sure but I think we tried to respect it in immerflow. @andrea-iob , correct me if I'm wrong.
Do we want to change this style?

@roccoarpa
Copy link
Contributor

We have a code style guide here. @roccoarpa , I'm not sure but I think we tried to respect it in immerflow. @andrea-iob , correct me if I'm wrong. Do we want to change this style?

Sorry, my bad, i totally misunderstood. Please forget my previous comment (i'm going to delete it). Please proceed how you feel more appropriate.

@roccoarpa roccoarpa removed their request for review April 21, 2022 16:34
@edoardolombardi
Copy link
Member

Cool. Thanks @andrea-iob, I think this tool should be useful for all our developments.

Sorry, my bad, i totally misunderstood. Please forget my previous comment (i'm going to delete it). Please proceed how you feel more appropriate.

Maybe I'm wrong, but I think that your deleted comment @roccoarpa was not off topic.

In particular referring to

We have a code style guide here. @roccoarpa , I'm not sure but I think we tried to respect it in immerflow. @andrea-iob , correct me if I'm wrong. Do we want to change this style?

a couple of questions:

  • the new clang-format style file is completely compliant with the guide style currently used in bitpit?
  • this clang-format style introduces new style definitions not already included in the style guide?

If not, I agree with @roccoarpa, maybe an internal meeting can be useful to inform all the developers, to discuss and to agree on a common coding style and to adopt it in all our Optimad codes.

PS: I noticed that @andrea-iob already started a chat with some of us in this direction.

@andrea-iob andrea-iob force-pushed the bitpit.automatic.formatting branch 2 times, most recently from 188217b to 7889de9 Compare April 29, 2022 07:00
@andrea-iob
Copy link
Member Author

I've enabled alignment of consecutive macros, assignments, and bitfields (AlignConsecutiveMacros, AlignConsecutiveAssignments, AlignConsecutiveBitFields).

@roccoarpa
Copy link
Contributor

roccoarpa commented May 11, 2022

Hello there: we run some test with the new formatting together with @kgkara, and the current format layout (with = alignment active) is fine for us. Anyway, I want to point out the question of the version of clang-format. I don't remember with which version this .clang-format yaml file is compliant. I used the version 12 (on Ubuntu newer version are not available with apt for now) and this file triggers error of unrecognized keys. I transferred the modifications on a clang12 compliant yaml (reported below) and this works.

Let us know which version of clang-format will be officially supported in the end.

Language:        Cpp
# BasedOnStyle:  LLVM
AccessModifierOffset: -2
AlignAfterOpenBracket: Align
AlignConsecutiveMacros: Consecutive
AlignConsecutiveAssignments: Consecutive
AlignConsecutiveBitFields: Consecutive
AlignConsecutiveDeclarations: None
AlignEscapedNewlines: Right
AlignOperands:   Align
AlignTrailingComments: true
AllowAllArgumentsOnNextLine: true
AllowAllConstructorInitializersOnNextLine: true
AllowAllParametersOfDeclarationOnNextLine: true
AllowShortEnumsOnASingleLine: true
AllowShortBlocksOnASingleLine: Never
AllowShortCaseLabelsOnASingleLine: false
AllowShortFunctionsOnASingleLine: All
AllowShortLambdasOnASingleLine: All
AllowShortIfStatementsOnASingleLine: Never
AllowShortLoopsOnASingleLine: false
AlwaysBreakAfterDefinitionReturnType: None
AlwaysBreakAfterReturnType: None
AlwaysBreakBeforeMultilineStrings: false
AlwaysBreakTemplateDeclarations: MultiLine
AttributeMacros:
  - __capability
BinPackArguments: true
BinPackParameters: true
BraceWrapping:
  AfterCaseLabel:  true
  AfterClass:      true
  AfterControlStatement: Never
  AfterEnum:       true
  AfterFunction:   true
  AfterNamespace:  false
  AfterObjCDeclaration: false
  AfterStruct:     true
  AfterUnion:      true
  AfterExternBlock: false
  BeforeCatch:     false
  BeforeElse:      false
  BeforeLambdaBody: true
  BeforeWhile:     false
  IndentBraces:    false
  SplitEmptyFunction: true
  SplitEmptyRecord: true
  SplitEmptyNamespace: true
BreakBeforeBinaryOperators: None
BreakBeforeConceptDeclarations: true
BreakBeforeBraces: Custom
BreakBeforeInheritanceComma: false
BreakInheritanceList: BeforeColon
BreakBeforeTernaryOperators: true
BreakConstructorInitializersBeforeComma: false
BreakConstructorInitializers: BeforeColon
BreakAfterJavaFieldAnnotations: false
BreakStringLiterals: true
ColumnLimit:     0
CommentPragmas:  '^ IWYU pragma:'
CompactNamespaces: false
ConstructorInitializerAllOnOneLineOrOnePerLine: false
ConstructorInitializerIndentWidth: 4
ContinuationIndentWidth: 4
Cpp11BracedListStyle: true
DeriveLineEnding: true
DerivePointerAlignment: false
DisableFormat:   false
EmptyLineBeforeAccessModifier: LogicalBlock
ExperimentalAutoDetectBinPacking: false
FixNamespaceComments: true
ForEachMacros:
  - foreach
  - Q_FOREACH
  - BOOST_FOREACH
StatementAttributeLikeMacros:
  - Q_EMIT
IncludeBlocks:   Preserve
IncludeCategories:
  - Regex:           '^"(llvm|llvm-c|clang|clang-c)/'
    Priority:        2
    SortPriority:    0
    CaseSensitive:   false
  - Regex:           '^(<|"(gtest|gmock|isl|json)/)'
    Priority:        3
    SortPriority:    0
    CaseSensitive:   false
  - Regex:           '.*'
    Priority:        1
    SortPriority:    0
    CaseSensitive:   false
IncludeIsMainRegex: '(Test)?$'
IncludeIsMainSourceRegex: ''
IndentCaseLabels: false
IndentCaseBlocks: false
IndentGotoLabels: true
IndentPPDirectives: None
IndentExternBlock: AfterExternBlock
IndentRequires:  false
IndentWidth:     4
IndentWrappedFunctionNames: false
InsertTrailingCommas: None
JavaScriptQuotes: Leave
JavaScriptWrapImports: true
KeepEmptyLinesAtTheStartOfBlocks: false
MacroBlockBegin: ''
MacroBlockEnd:   ''
MaxEmptyLinesToKeep: 1
NamespaceIndentation: None
ObjCBinPackProtocolList: Auto
ObjCBlockIndentWidth: 2
ObjCBreakBeforeNestedBlockParam: true
ObjCSpaceAfterProperty: false
ObjCSpaceBeforeProtocolList: true
PenaltyBreakAssignment: 2
PenaltyBreakBeforeFirstCallParameter: 19
PenaltyBreakComment: 300
PenaltyBreakFirstLessLess: 120
PenaltyBreakString: 1000
PenaltyBreakTemplateDeclaration: 10
PenaltyExcessCharacter: 1000000
PenaltyReturnTypeOnItsOwnLine: 60
PenaltyIndentedWhitespace: 0
PointerAlignment: Right
ReflowComments:  true
SortIncludes:    true
SortJavaStaticImport: Before
SortUsingDeclarations: true
SpaceAfterCStyleCast: true
SpaceAfterLogicalNot: false
SpaceAfterTemplateKeyword: true
SpaceBeforeAssignmentOperators: true
SpaceBeforeCaseColon: false
SpaceBeforeCpp11BracedList: false
SpaceBeforeCtorInitializerColon: true
SpaceBeforeInheritanceColon: true
SpaceBeforeParens: ControlStatements
SpaceAroundPointerQualifiers: Default
SpaceBeforeRangeBasedForLoopColon: true
SpaceInEmptyBlock: false
SpaceInEmptyParentheses: false
SpacesBeforeTrailingComments: 1
SpacesInAngles:  false
SpacesInConditionalStatement: false
SpacesInContainerLiterals: true
SpacesInCStyleCastParentheses: false
SpacesInParentheses: false
SpacesInSquareBrackets: false
SpaceBeforeSquareBrackets: false
BitFieldColonSpacing: Both
Standard:        Latest
StatementMacros:
  - Q_UNUSED
  - QT_REQUIRE_VERSION
TabWidth:        4
UseCRLF:         false
UseTab:          Never
WhitespaceSensitiveMacros:
  - STRINGIZE
  - PP_STRINGIZE
  - BOOST_PP_STRINGIZE
  - NS_SWIFT_NAME
  - CF_SWIFT_NAME
... 

@andrea-iob
Copy link
Member Author

Let us know which version of clang-format will be officially supported in the end.

Let's use version 12. I will replace the .clang-format file with yours.

@andrea-iob andrea-iob force-pushed the bitpit.automatic.formatting branch from 7889de9 to 8efb13c Compare May 12, 2022 07:11
@andrea-iob
Copy link
Member Author

I've updated the .clang-format file.

@andrea-iob andrea-iob force-pushed the bitpit.automatic.formatting branch 3 times, most recently from c6df8ca to 494ad7e Compare May 12, 2022 13:40
@andrea-iob
Copy link
Member Author

andrea-iob commented May 12, 2022

For each module/test I added a target named clang-format-<name> that will format all the sources of that module/test. There is also a global target, called clang-format, that depends on all other clang-format targets.

For each module/test a target named clang-format-<name> is added.
There is also a global target, called clang-format, that depends
on all other clang-format targets.
@andrea-iob
Copy link
Member Author

I updated the style guide and I did the following changes to the configuration file:

 ---
 Language:        Cpp
 # BasedOnStyle:  LLVM
-AccessModifierOffset: -2
+AccessModifierOffset: -4
 AlignAfterOpenBracket: Align
 AlignConsecutiveMacros: Consecutive
 AlignConsecutiveAssignments: Consecutive
 AlignConsecutiveBitFields: Consecutive
 AlignConsecutiveDeclarations: None
 AlignEscapedNewlines: Right
 AlignOperands:   Align
-AlignTrailingComments: true
+AlignTrailingComments: false
 AllowAllArgumentsOnNextLine: true
 AllowAllConstructorInitializersOnNextLine: true
 AllowAllParametersOfDeclarationOnNextLine: true
 AllowShortEnumsOnASingleLine: true
 AllowShortBlocksOnASingleLine: Never
@@ -121,17 +121,17 @@ PenaltyBreakString: 1000
 PenaltyBreakTemplateDeclaration: 10
 PenaltyExcessCharacter: 1000000
 PenaltyReturnTypeOnItsOwnLine: 60
 PenaltyIndentedWhitespace: 0
 PointerAlignment: Right
-ReflowComments:  true
+ReflowComments:  false
 SortIncludes:    true
 SortJavaStaticImport: Before
 SortUsingDeclarations: true
 SpaceAfterCStyleCast: true
 SpaceAfterLogicalNot: false
-SpaceAfterTemplateKeyword: true
+SpaceAfterTemplateKeyword: false
 SpaceBeforeAssignmentOperators: true
 SpaceBeforeCaseColon: false
 SpaceBeforeCpp11BracedList: false
 SpaceBeforeCtorInitializerColon: true
 SpaceBeforeInheritanceColon: true

If there are no objections, I'm going to merge this pull request this week. After that, I will create additional pull requests for enforcing the coding style of the single bitpit modules.

@andrea-iob andrea-iob merged commit 9765f09 into master Sep 16, 2022
@andrea-iob andrea-iob deleted the bitpit.automatic.formatting branch September 16, 2022 06:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants