Conversation
jjuhl
force-pushed
the
clang-format
branch
2 times, most recently
from
9d5a0ef to
5a41422
Compare
|
The forced push just rebased the branch on the current state of 1.x . |
|
I finally found the time to review this. First of all, thanks a lot for this, it's a big step forwards in improving future code quality. There are many changes that I don't really like, but that was to be expected: consistency requires fixed rules instead of rules that vary depending on circumstances. You did do a great job at keeping the style close, I don't think there is anything in the diff that I would absolutely hate to merge. I did make a few additional tweaks though. Unless you have certain reasons why existing options might be better, I would make the following changes to the .clang-format file: diff --git a/.clang-format b/.clang-format
index e8f91972..2a1be1ff 100644
--- a/.clang-format
+++ b/.clang-format
@@ -85,8 +85,6 @@ AllowShortLoopsOnASingleLine: false
AllowShortNamespacesOnASingleLine: false
AlwaysBreakAfterDefinitionReturnType: None
AlwaysBreakBeforeMultilineStrings: true
-AttributeMacros:
- - __capability
BinPackArguments: false
BinPackLongBracedList: true
BinPackParameters: OnePerLine
@@ -103,7 +101,7 @@ BraceWrapping:
AfterObjCDeclaration: false
AfterStruct: true
AfterUnion: true
- BeforeCatch: false
+ BeforeCatch: true
BeforeElse: true
BeforeLambdaBody: true
BeforeWhile: false
@@ -116,7 +114,7 @@ BreakAfterAttributes: Leave
BreakAfterJavaFieldAnnotations: false
BreakAfterReturnType: None
BreakArrays: true
-BreakBeforeBinaryOperators: None
+BreakBeforeBinaryOperators: NonAssignment
BreakBeforeConceptDeclarations: Always
BreakBeforeBraces: Custom
BreakBeforeInlineASMColon: OnlyMultiline
@@ -128,7 +126,7 @@ BreakFunctionDefinitionParameters: false
BreakInheritanceList: AfterColon
BreakStringLiterals: true
BreakTemplateDeclarations: Yes
-ColumnLimit: 130
+ColumnLimit: 135
CommentPragmas: '^ IWYU pragma:'
CompactNamespaces: false
ConstructorInitializerIndentWidth: 4
@@ -141,40 +139,32 @@ EmptyLineBeforeAccessModifier: Always
EnumTrailingComma: Leave
ExperimentalAutoDetectBinPacking: false
FixNamespaceComments: true
-ForEachMacros:
- - foreach
- - Q_FOREACH
- - BOOST_FOREACH
-IfMacros:
- - KJ_IF_MAYBE
-IncludeBlocks: Preserve
-IncludeCategories: []
-# # Other TGUI headers (everything in <TGUI/> except extlibs)
-# - Regex: '^<TGUI/(?!extlibs/).*>'
-# Priority: 1
-# CaseSensitive: false
-# # Known third-party headers
-# # List all known prefixes so standard library headers are not caught here
-# - Regex: '^<TGUI/extlibs/.*>'
-# Priority: 2
-# CaseSensitive: false
-# - Regex: '^<SFML/.*>'
-# Priority: 2
-# CaseSensitive: false
-# - Regex: '^<SDL/.*>'
-# Priority: 2
-# CaseSensitive: false
-# - Regex: '^<SDL3/.*>'
-# Priority: 2
-# CaseSensitive: false
-# - Regex: '^<RAYLIB/.*>'
-# Priority: 2
-# CaseSensitive: false
-# # Standard library headers
-# # Anything not matched by previous categories
-# - Regex: '^<.*>'
-# Priority: 3
-# CaseSensitive: false
+IncludeBlocks: Regroup
+IncludeCategories:
+ - Regex: '^\"(.+)\"$'
+ Priority: 1
+ - Regex: '^<TGUI/Config.hpp>$'
+ Priority: 2
+ - Regex: '^<TGUI/Global.hpp>$'
+ Priority: 2
+ - Regex: '^<TGUI/TGUI.hpp>$'
+ Priority: 2
+ - Regex: '^<TGUI/extlibs/.*>$'
+ Priority: 3
+ - Regex: '^<TGUI/Backend/.*>$'
+ Priority: 4
+ - Regex: '^<TGUI/.*>$'
+ Priority: 5
+ - Regex: '^<SFML/.*>$'
+ Priority: 6
+ - Regex: '^<SDL/.*>$'
+ Priority: 6
+ - Regex: '^<SDL3/.*>$'
+ Priority: 6
+ - Regex: '^<RAYLIB/.*>$'
+ Priority: 6
+ - Regex: '^<.+>$'
+ Priority: 7
IncludeIsMainRegex: '\.hpp$'
IncludeIsMainSourceRegex: '\.cpp$'
IndentAccessModifiers: false
@@ -183,7 +173,7 @@ IndentCaseLabels: true
IndentExportBlock: true
IndentExternBlock: Indent
IndentGotoLabels: true
-IndentPPDirectives: None
+IndentPPDirectives: BeforeHash
IndentRequiresClause: true
IndentWidth: 4
IndentWrappedFunctionNames: true
@@ -217,7 +207,7 @@ ObjCBreakBeforeNestedBlockParam: true
ObjCSpaceAfterProperty: false
ObjCSpaceBeforeProtocolList: true
OneLineFormatOffRegex: ''
-PackConstructorInitializers: NextLine
+PackConstructorInitializers: Never
PenaltyBreakAssignment: 2000
PenaltyBreakBeforeFirstCallParameter: 2000
PenaltyBreakBeforeMemberAccess: 150
@@ -252,8 +242,8 @@ SeparateDefinitionBlocks: Leave
ShortNamespaceLines: 1
SkipMacroDefinitionBody: false
SortIncludes:
- Enabled: false
- IgnoreCase: false
+ Enabled: true
+ IgnoreCase: true
SortJavaStaticImport: Before
SortUsingDeclarations: LexicographicNumeric
SpaceAfterCStyleCast: false
@@ -267,7 +257,7 @@ SpaceBeforeCpp11BracedList: false
SpaceBeforeCtorInitializerColon: true
SpaceBeforeInheritanceColon: true
SpaceBeforeJsonColon: false
-SpaceBeforeParens: ControlStatements
+SpaceBeforeParens: Custom
SpaceBeforeParensOptions:
AfterControlStatements: true
AfterForeachMacros: true
@@ -297,21 +287,10 @@ SpacesInParensOptions:
InEmptyParentheses: false
Other: false
SpacesInSquareBrackets: false
-Standard: c++14
-StatementAttributeLikeMacros:
- - Q_EMIT
-StatementMacros:
- - Q_UNUSED
- - QT_REQUIRE_VERSION
+Standard: c++20
TableGenBreakInsideDAGArg: DontBreak
TabWidth: 4
UseTab: Never
VerilogBreakBetweenInstancePorts: true
-WhitespaceSensitiveMacros:
- - BOOST_PP_STRINGIZE
- - CF_SWIFT_NAME
- - NS_SWIFT_NAME
- - PP_STRINGIZE
- - STRINGIZE
WrapNamespaceBodyWithEmptyLines: Never
...Do you know of any downside in having Standard set to c++20 even though the code mainly focuses on c++14? There is some conditional c++20 code that seems better formatted when setting the Standard to c++20 (in c++14 mode it is turning With my local build settings there is no issue with tests/String.cpp when it is formatted. Maybe you can remove the commit that disables formatting in that file and then I might be able to find the issue based on CI output? If you ever have unexplained failures again, feel free to just leave them in and let CI fail. I'd rather hunt for an issue than having code with a workaround where I don't understood why the workaround is needed. include/TGUI/Config.hpp.in can probably also be updated. It's not a valid c++ file and clang-format would insert spaces before @ signs, which breaks the file. However the other changes suggested by clang-format can probably be kept. The command I ended up using to run clang-format on valid files was the following. The find . -type f \( -name '*.cpp' -o -name '*.hpp' \) ! -path '*/build*' ! -path '*/.cxx/*' ! -path './include/TGUI/extlibs/*' ! -path './tests/catch.hpp' -print0 | xargs -0 clang-format --style=file -iIf you update your PR with the suggested changes then I will scroll though the diff again. Maybe I'll notice some other things that need to be tweaked. For some of the suggestions I made I only briefly looked at the results, so maybe I'll even decide to undo some changes when I see how it acts on different parts of the code. |
|
Thanks a lot for the feedback. I'll update the PR with your changes and drop the commit for tests/String.cpp (after your changes everything actually builds for me without that commit - I haven't dug into it, but I suspect it's maybe the C++20 change (or maybe my local build config was just wonky when I initially did this)).. I'm very curious to see what impact your changes to the include ordering will have. I didn't manage to get to a point where cpp files would still include their own headers first in all cases and also do the sorting in your preferred order - if you've managed that here then there's something for me to learn (which is always nice). The "BeforeCatch" change is a nice catch, I don't know how I missed that. I'll push an updated branch later tonight. |
|
PR updated with your changes :-) |
I didn't manage it either, I just gave up on that when I saw that SFML wasn't doing that either. The sorting order is the last thing that I changed and probably the one that I paid the least attention to. I very briefly scrolled over the new diff and it looks pretty good. The only mistake I saw so far was that |
|
Alright, I'll take another look at this tomorrow to see if I can improve it. |
|
Hi Texus I've just updated the PR and I've made 2 changes:
and I think that's slightly better, but more importantly it's a better default for the future (IMHO). PR updated with those minor changes. As I've hinted about previously, there are definitely style changes that I would like to make in this .clang-format file, but I've not done so. The most important thing right now is to just get clang-format introduced with a sane baseline that doesn't change too much. Future PR's can then discuss the value of changing one or more options. I'll also be submitting a PR to format DefaultFont.hpp once this PR is merged, but I don't want to include it here since it would just drown out the other changes. Thoughts? Feedback? Kind regards, |
|
Maybe the includes for GLFW, SDL_ttf and raylib can also be sorted as being dependencies like in the patch below. Afterwards I will merge this PR. diff --git a/.clang-format b/.clang-format
index ebaf232e..cd688cbe 100644
--- a/.clang-format
+++ b/.clang-format
@@ -157,9 +157,13 @@ IncludeCategories:
Priority: 5
- Regex: '^<SFML/.*>$'
Priority: 6
- - Regex: '^<SDL[0-9]?/.*>$'
+ - Regex: '^<GLFW/.*>$'
Priority: 6
- - Regex: '^<RAYLIB/.*>$'
+ - Regex: '^<SDL.*>$'
+ Priority: 6
+ - Regex: '^<raylib.h>$'
+ Priority: 6
+ - Regex: '^<rlgl.h>$'
Priority: 6
- Regex: '^<.+>$'
Priority: 7Maybe something like following can also be added to the changelog. This is only one of the many changes you are making, but this PR feels big/important enough for me to be individually linked. |
Reformatting this huge chunk of essentially raw data dwarfs and obscures all the other formatting changes clang-tidy will make and makes for a very messy commit introducing the initial formatting, so for now we'll tell clang-format to ignore most of the file. We can still re-format this file later if we want to, as its own single commit (and I personally believe we do want that, but that's for later).
This file started life as a copy of the .clang-format file from SFML
with additional clang-format defaults added by doing
clang-format --style=file --dump-config
The resulting file was then tweaked over multiple iterations to
minimize the diff generated when applying it.
Also tweaked a bit by applying changes suggested by Texus in the
initial PR.
The intention here is to start off with a .clang-format rules file
that matches the existing style as closely as possible. That's not to
say that we don't want to tweak this further in the future (I
absolutely believe we do want to do that) but we want to start off
with minimal changes and just get to a point where we can start using
clang format in IDE's and editors (and maybe even (later) git
pre-commit hooks or CI checks to enforce style) to enforce a
consistent style, with minimal friction to adoption.
Style changes beyond this starting point can be added gradually one by
one.
This applies the current .clang-format file to the entire source tree.
There are compromises here, it's impossible to create a rules file
that applies globally that doesn't involve some compromises, but this
is my best effort at matching the existing style and applying sensible
rules while not completely neutering clang-format.
This was done by
find . -type f \( -name '*.cpp' -o -name '*.hpp' \) ! -path '*/build*' ! -path '*/.cxx/*' ! -path './include/TGUI/extlibs/*' ! -path './tests/catch.hpp' -print0 | xargs -0 clang-format --style=file -i
Run
clang-format --style=file -i include/TGUI/Config.hpp.in
then manually undo the incorrect changes that clang-format makes since
it is not a valid C++ source file.
This gets the C++ parts of the file consistently formated with the
rest of the code.
|
Updated as requested and rebased on current 1.x |
2 participants
Hi Texus
Here's my current best bet at introducing clang-format to the TGUI code base.
Please read the commit comments as I've tried to explain there why I've done what I did (and some details on compromises etc).
I tried my hardest to match the current style as best I could. But since the style is inconsistent and not enforced currently there was no way I could create a rules file with zero changes.
But, what I ended up with is pretty good IMHO. I managed to not make huge changes to all files and for the files that are changed I think the changes mostly make sense. There are of course going to be some changes in some files that locally may not seem the best, but they are then at least globally consistent. There's always going to have to be compromises when you do something like this.
If there are style changes in this PR that you just absolutely cannot accept or just hate for various reasons, then let me know - perhaps I can tweak the rules to match what you want or just tell clang-format to leave those lines alone. I'll do my best to match what you want.
In the future you may see PR's from me suggesting style changes - we can discuss those one at a time when they arrive. The important thing right now is to establish a baseline that can be enforced going forward.
Any comments and feedback welcome.
I don't expect you to merge this PR as-is. Its purpose is to be the start of a discussion (but if you love it as-is, feel free to merge of course).
Kind regards,
Jesper Juhl