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

Add clang-format configuration #1585

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

IvanSavenko
Copy link
Member

Second attempt at clang-format, slightly improved & corrected version compared to previous PR.

It is still not perfect, but I've been using it recently with quite good results. So proposing to add it to our repo and use it as guideline for formatting.

I do not think that clang-format is good enough for automatic usage, e.g. in that git+clang-format tool but only as a base guideline. Meaning, code still needs basic readability pass by a developer, however with clang-format you can forget about "basic" stuff like spacing.

Example of such usage is my PR #1581, for example these two files:
https://github.com/IvanSavenko/vcmi/blob/map_render_rewrite/client/adventureMap/MapRenderer.cpp
https://github.com/IvanSavenko/vcmi/blob/map_render_rewrite/client/adventureMap/MapRenderer.h

I think at this point we can add it to repo, let everyone else try it and do any further changes based on everyone's opinion.

@rilian-la-te
Copy link
Contributor

@IvanSavenko please, consider this differences:

Language:        Cpp
# BasedOnStyle:  Mozilla
AccessModifierOffset: -4
AlignAfterOpenBracket: Align
AlignArrayOfStructures: Left
AlignConsecutiveMacros: false 
AlignConsecutiveAssignments: false 
AlignConsecutiveBitFields: false 
AlignConsecutiveDeclarations: false
AlignEscapedNewlines: Left
AlignOperands:   AlignAfterOperator
AlignTrailingComments: false
AllowAllArgumentsOnNextLine: true
AllowAllParametersOfDeclarationOnNextLine: false
AllowShortEnumsOnASingleLine: false
AllowShortBlocksOnASingleLine: Never
AllowShortCaseLabelsOnASingleLine: false
AllowShortFunctionsOnASingleLine: Inline
AllowShortLambdasOnASingleLine: All
AllowShortIfStatementsOnASingleLine: Never
AllowShortLoopsOnASingleLine: false
AlwaysBreakAfterDefinitionReturnType: None
AlwaysBreakAfterReturnType: None
AlwaysBreakBeforeMultilineStrings: true
AlwaysBreakTemplateDeclarations: Yes
AttributeMacros:
  - __capability
BinPackArguments: false
BinPackParameters: false
BraceWrapping:
  AfterCaseLabel:  false
  AfterClass:      true
  AfterControlStatement: Never
  AfterEnum:       true
  AfterFunction:   true
  AfterNamespace:  false
  AfterObjCDeclaration: false
  AfterStruct:     true
  AfterUnion:      true
  AfterExternBlock: true
  BeforeCatch:     false
  BeforeElse:      false
  BeforeLambdaBody: false
  BeforeWhile:     false
  IndentBraces:    false
  SplitEmptyFunction: true
  SplitEmptyRecord: false
  SplitEmptyNamespace: true
BreakBeforeBinaryOperators: None
BreakBeforeConceptDeclarations: true
BreakBeforeBraces: Allman
BreakBeforeInheritanceComma: true
BreakInheritanceList: AfterColon
BreakBeforeTernaryOperators: true
BreakConstructorInitializers: AfterColon
BreakAfterJavaFieldAnnotations: false
BreakStringLiterals: true
ColumnLimit:     160
CommentPragmas:  '^ IWYU pragma:'
QualifierAlignment: Left
CompactNamespaces: false
ConstructorInitializerIndentWidth: 4
ContinuationIndentWidth: 4
Cpp11BracedListStyle: true
DeriveLineEnding: true
DerivePointerAlignment: false
DisableFormat:   false
EmptyLineAfterAccessModifier: Never
EmptyLineBeforeAccessModifier: LogicalBlock
ExperimentalAutoDetectBinPacking: false
PackConstructorInitializers: BinPack
BasedOnStyle:    ''
ConstructorInitializerAllOnOneLineOrOnePerLine: false
FixNamespaceComments: false
ForEachMacros:
  - foreach
  - Q_FOREACH
  - BOOST_FOREACH
IfMacros:
  - KJ_IF_MAYBE
IncludeBlocks: Preserve
IncludeCategories:
  # Precompiled header
  - Regex:           '"StdInc\.h"'
    Priority:        0
    SortPriority:    -1
IncludeIsMainRegex: '(Test)?$'
IncludeIsMainSourceRegex: ''
IndentAccessModifiers: false
IndentCaseLabels: true
IndentCaseBlocks: false
IndentGotoLabels: true
IndentPPDirectives: AfterHash
IndentExternBlock: AfterExternBlock
IndentRequires:  false
IndentWidth:     4
IndentWrappedFunctionNames: false
InsertTrailingCommas: None
JavaScriptQuotes: Leave
JavaScriptWrapImports: true
KeepEmptyLinesAtTheStartOfBlocks: true
LambdaBodyIndentation: Signature
MacroBlockBegin: ''
MacroBlockEnd:   ''
MaxEmptyLinesToKeep: 1
NamespaceIndentation: None
ObjCBinPackProtocolList: Auto
ObjCBlockIndentWidth: 4
ObjCBreakBeforeNestedBlockParam: true
ObjCSpaceAfterProperty: true
ObjCSpaceBeforeProtocolList: false
PackConstructorInitializers: CurrentLine
PenaltyBreakAssignment: 2
PenaltyBreakBeforeFirstCallParameter: 19
PenaltyBreakComment: 300
PenaltyBreakFirstLessLess: 120
PenaltyBreakOpenParenthesis: 0
PenaltyBreakString: 1000
PenaltyBreakTemplateDeclaration: 10
PenaltyExcessCharacter: 1000000
PenaltyReturnTypeOnItsOwnLine: 200
PenaltyIndentedWhitespace: 0
PointerAlignment: Middle
PPIndentWidth:   -1
ReferenceAlignment: Pointer
ReflowComments:  false
RemoveBracesLLVM: false
SeparateDefinitionBlocks: Leave
ShortNamespaceLines: 1
SortIncludes:    CaseSensitive
SortJavaStaticImport: Before
SortUsingDeclarations: true
SpaceAfterCStyleCast: false
SpaceAfterLogicalNot: false
SpaceAfterTemplateKeyword: false
SpaceBeforeAssignmentOperators: true
SpaceBeforeCaseColon: false
SpaceBeforeCpp11BracedList: false
SpaceBeforeCtorInitializerColon: false
SpaceBeforeInheritanceColon: false
SpaceBeforeParens: Never
SpaceBeforeParensOptions:
  AfterControlStatements: true
  AfterForeachMacros: true
  AfterFunctionDefinitionName: false
  AfterFunctionDeclarationName: false
  AfterIfMacros:   true
  AfterOverloadedOperator: false
  BeforeNonEmptyParentheses: false
SpaceAroundPointerQualifiers: Default
SpaceBeforeRangeBasedForLoopColon: true
SpaceInEmptyBlock: false
SpaceInEmptyParentheses: false
SpacesBeforeTrailingComments: 1
SpacesInAngles:  Never
SpacesInConditionalStatement: false
SpacesInContainerLiterals: true
SpacesInCStyleCastParentheses: false
SpacesInLineCommentPrefix:
  Minimum:         0
  Maximum:         -1
SpacesInParentheses: false
SpacesInSquareBrackets: false
SpaceBeforeSquareBrackets: false
BitFieldColonSpacing: Both
Standard:        Latest
StatementAttributeLikeMacros:
  - FALLTHROUGH
  - STRONG_INLINE
  - DLL_EXPORT
  - DLL_IMPORT
  - ELF_VISIBILITY
  - Q_EMIT
StatementMacros:
  - Q_UNUSED
  - QT_REQUIRE_VERSION
TabWidth:        4
UseCRLF:         false
UseTab:          ForContinuationAndIndentation 
WhitespaceSensitiveMacros:
  - STRINGIZE
  - PP_STRINGIZE
  - BOOST_PP_STRINGIZE
  - NS_SWIFT_NAME
  - CF_SWIFT_NAME

@IvanSavenko
Copy link
Member Author

@rilian-la-te
Difference between our files: (my version | your version)
Looking into their effect...

AllowShortFunctionsOnASingleLine: Empty | Inline
AllowShortLambdasOnASingleLine: None | All

SplitEmptyFunction: false | true
SplitEmptyNamespace false | true

BreakBeforeInheritanceComma: false | true
BreakInheritanceList: BeforeComma | AfterColon
BreakConstructorInitializersBeforeComma: false | (not set)
BreakConstructorInitializers: BeforeComma | AfterColon
AllowAllConstructorInitializersOnNextLine: true | false
PackConstructorInitializers: (not set) | CurrentLine
SpaceBeforeCtorInitializerColon: true | false
SpaceBeforeInheritanceColon: true | false

@IvanSavenko
Copy link
Member Author

@rilian-la-te
Comparison result (check https://clang.llvm.org/docs/ClangFormatStyleOptions.html if anyone want to see rule effect)

  • AllowShortFunctionsOnASingleLine: Empty | Inline - Keeping mine, I think we only use this form for empty implementation, e.g. interface
  • AllowShortLambdasOnASingleLine: None | All - Keeping mine, I think we never use single-line lambda form
  • SplitEmptyFunction: false | true - No strong opinion here, if somebody specifically prefers one style or other let me know. Otherwise keeping mine
  • SplitEmptyNamespace false | true - No strong opinion here. And most likely unused in our code, since we barely use namespaces. Changed for consistency with other similar rules.
  • BreakBeforeInheritanceComma: false | true - For some reason, not documented in clang-format 7+. Removed? In any case, our style is probably "false"
  • BreakInheritanceList: BeforeComma | AfterColon - We generally use neither, and always keep inheritance list in single line, so does not matters I guess?
  • BreakConstructorInitializersBeforeComma: false | (not set) - looks like typo in my config? Will remove I guess?
  • BreakConstructorInitializers: BeforeComma | AfterColon - I slightly prefer this style, I believe same goes for @kambala-decapitator . Not against using AfterColon form, but if nobody insist on this form I'd rather stick with BeforeComma
  • AllowAllConstructorInitializersOnNextLine: true | false - deprecated, but false seems to match our style, will change
  • PackConstructorInitializers: (not set) | CurrentLine - too new for my clang-format, but I think we should use Never
  • SpaceBeforeCtorInitializerColon: true | false - probably no effect with my current style? But old code uses false, will change
  • SpaceBeforeInheritanceColon: true | false - from what I see in code, our style uses "true"

@rilian-la-te
Copy link
Contributor

@IvanSavenko

About 'BreakConstructorInitializersBeforeComma' - for new clang-format it is deprecated and replaced by BreakConstructorInitializers. I strongly prefer AfterColon style, it just looks more natural for me.

@rilian-la-te
Copy link
Contributor

About SplitEmptyFunction - it seems than old code uses True.

@kambala-decapitator
Copy link
Collaborator

my personal preferences on the differences:

AllowShortFunctionsOnASingleLine: Empty
AllowShortLambdasOnASingleLine: Inline or All
SplitEmptyFunction: true
SplitEmptyNamespace: true
BreakConstructorInitializers: BeforeComma
AllowAllConstructorInitializersOnNextLine: false
PackConstructorInitializers: CurrentLine
SpaceBeforeCtorInitializerColon: true
SpaceBeforeInheritanceColon: true

BreakBeforeInheritanceComma: no idea as not in docs. Based on the name it's about

class A : public B, public C {};
// for consistency we may use same as BreakConstructorInitializers
class A : public B
  , public C
{
};
// or even
class A
  : public B
  , public C
{
};

but at the same time multiple inheritance is a rather rare thing (and adding new parent is even rarer) and would be fine to just have all parents on a single line.

@IvanSavenko IvanSavenko marked this pull request as draft July 18, 2023 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants