gil icon indicating copy to clipboard operation
gil copied to clipboard

Unified formatting through clang-format

Open simmplecoder opened this issue 4 years ago • 11 comments

Is your feature request related to a problem? Please describe.

During PRs developers spend time on making formatting consistent across the project. That takes time and a thing that could be totally automated.

Describe the solution you'd like

Add a .clang-format file that would dictate how files are formatted, and possibly create a hook so that it gets reformatted automatically

Describe alternatives you've considered

I haven't found any formatting engine that would be as good as clang-format. Other IDEs either have their proprietary version or have moved to clang-format

Proposed .clang-format

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

I have tried this out, and it works for our codebase. The only problem being BeforeLambdaBody is introduced in clang-format 12 I believe, which is yet to be released.

simmplecoder avatar Aug 27 '20 09:08 simmplecoder

First, I'm a huge supporter of incorporating clang-format. I'm happy to be not a lonely one :)

Have you seen these?

mloskot avatar Aug 27 '20 10:08 mloskot

Perhaps I could submit a PR then to update the file, and the people who want it will just ln -s $PWD/example/clang-format/.clang-format $PWD/.clang-format ?

I believe that would be a reasonable tradeoff. @lpranam @codejaeger @stefanseefeld what do you think?

Perhaps we could try testing it on latest PRs and put it at the root of the project?

simmplecoder avatar Aug 27 '20 10:08 simmplecoder

I personally give

  • higher priority to consistent coding style and formatting throughout the whole codebase
  • lower priority to what particular coding style is used

As long as the coding style makes code readable, compact, horizontally limited, I'm fine with it.

The lack of consistency in GIL, especially in parts of IO, have been annoying, but manual improvements and maintenance cost lots of time. Hence, I'm eager to require use of clang-format, even if I need to trade personal preferences :-)

mloskot avatar Aug 27 '20 10:08 mloskot

Perhaps we could try testing it on latest PRs and put it at the root of the project?

I agree with this. At least we can have consistent styling in new code.

lpranam avatar Sep 05 '20 18:09 lpranam

I'd add to the overall discussion this RFC of mine https://trac.osgeo.org/geos/wiki/RFC4 where some details and implications are discussed, including external references, especially three parts of MongoDB story how they managed the "big reformat":

mloskot avatar Sep 08 '20 08:09 mloskot

We should give this a try :-)

https://www.youtube.com/watch?v=_EaU3VsDOJM

Clang Format Detector automatically finds the best matching Clang-Format Style for your code files.

mloskot avatar Oct 22 '20 10:10 mloskot

I am also all in for clang-format.

To avoid introducing a git diff barrier, making difficult to compare code before and after the "big reformat", we recently end-up rewriting the all repos history with git filter-repo. We kept "replace reference" for the old commits so that it did not break the reference to the commits in the master project (and since GIL is a git submodule for Boost we are in the same situation). Note that sometimes clang-format needs to be applied several times (or even does not converge!), aka first run leads some reformating, reapplying leads to more reformating. At least that's my experience with my repo.

sdebionne avatar Oct 22 '20 11:10 sdebionne

Once we agree on the definition of .clang-format, I don't mind either exploring the history rewrite option or just swallowing the "big reformat" pill.

mloskot avatar Oct 22 '20 12:10 mloskot

In past, I have used git filter-branch --tree-filter to reformate the entire code base while also maintaining the git commit history for blaming but I think this too would require force push and actually rewriting history. I have never used git filter-repo but I suspect it would be doing the same thing and would rewrite the history...

lpranam avatar Oct 22 '20 14:10 lpranam

Quoting the git filter-branch doc:

The git filter-repo tool is an alternative to git-filter-branch which does not suffer from these performance problems or the safety problems (mentioned below).

Is there any Boost wide .clang-format that we could use?

sdebionne avatar Oct 22 '20 16:10 sdebionne

@sdebionne

Is there any Boost wide .clang-format that we could use?

No, or I don't know about any, but some individual libraries have some (including us):

/d/boost.win (develop u= origin/develop) $ find . -name .clang-format
./libs/gil/example/clang-format/.clang-format
./libs/graph/.clang-format
./libs/histogram/.clang-format
./libs/multiprecision/.clang-format
./libs/nowide/.clang-format
./libs/numeric/ublas/.clang-format
./libs/outcome/.clang-format
./libs/yap/.clang-format

mloskot avatar Oct 22 '20 16:10 mloskot