add clang-format #4790

Disservin · 2023-09-16T14:57:29Z

Introduce clang-format to enforce a code style for Stockfish.
The lack of an automated code formatter has been a long standing issue #3608.
This PR includes a Makefile target to format the code accordingly.

closes #3608

Disservin · 2023-09-16T15:08:32Z

I might add that there have been previous pull requests with the same aim. This one aims to do it properly now, including a Makefile target.

The clang-format file is taken from my latest comment here, #3608.
Vondele and I have discussed a few aspects of the general code style that this introduces, see https://discord.com/channels/435943710472011776/813919248455827515/1152558142064963594.

I think this is a step in the right direction, though I think we need to

document how to install clang-format in our wiki, so that developers can make use of this
consider making this a step in our CI.
This could be either a simple fail red (with blocking the merge request) and/or
automatically add a formatter to the CI which pushes the new changes

peregrineshahin · 2023-09-16T15:21:09Z

I'm not a big fan of doing this on the Stockfish code, to me the formatter is still not smart enough to understand some intended formatting made by hand.
I see this as a regression on the developer experience, readability and, the aesthetic of the code (one example is that the conditions in if statements are not readable anymore), and the equal assignments beign aligned are yet to be smart to do that for += and -= in the evaluate.cpp file (L175, L176).
Sure there are problems in indentations that are discovered when applying this PR I would take the good changes and leave the ugly ones.

vondele · 2023-09-16T15:34:23Z

Personally, I'd be in favor of automatic formatting, removes some of the human burden to maintain it, and guarantees consistency. Sure, it looks different, but very readable, IMO.

src/Makefile

peregrineshahin · 2023-09-16T15:41:06Z

I'm unsure this is readable it looks very inconsistent to have && at the end on a line and sometimes at the beginning

            if (
              !PvNode && depth > 3 &&
              abs(beta) < VALUE_TB_WIN_IN_MAX_PLY
              // If value from transposition table is lower than probCutBeta, don't attempt probCut
              // there and in further interactions with transposition table cutoff depth is set to depth - 3
              // because probCut search has depth set to depth - 4 but we also do a move before it
              // So effective depth is equal to depth - 3
              && !(tte->depth() >= depth - 3 && ttValue != VALUE_NONE && ttValue < probCutBeta)) {

This also simply removes the emphasis of the importance of every condition and is just trying to fit as much is possible in one line

            // Step 9. Null move search with verification search (~35 Elo)
            if (!PvNode && (ss - 1)->currentMove != MOVE_NULL && (ss - 1)->statScore < 17329 &&
                eval >= beta && eval >= ss->staticEval &&
                ss->staticEval >= beta - 21 * depth + 258 && !excludedMove &&
                pos.non_pawn_material(us) && ss->ply >= thisThread->nmpMinPly &&
                beta > VALUE_TB_LOSS_IN_MAX_PLY) {

vondele · 2023-09-16T15:42:47Z

I'm unsure this is readable it looks very inconsistent to have && at the end on a line and sometimes at the beginning

rightful punishment for writing lines of comments between terms in a condition in an if statement ? ;-)

Disservin · 2023-09-16T15:44:12Z

I see this as a regression on the developer experience

I dont think it is, developers now have to worry about much less when writing code. One could consider moving this also into the build stage so that the github diff on fishtest is also (almost) always formatted.

the aesthetic of the code (one example is that the conditions in if statements are not readable anymore)

I think the main big conditional alignments that we have, are in search.cpp and these few conditions are a fraction of the entire codebase.
I would even be fine with dropping a few // clang-format off around some larger if conditions in the future.

vondele · 2023-09-16T15:47:35Z

I see this as a regression on the developer experience

I dont think it is, developers now have to worry about much less when writing code. One could consider moving this also into the build stage so that the github diff on fishtest is also (almost) always formatted.

Moving it in the build phase is not a good idea, it should be an explicit dev action to reformat (otherwise editors will see changing file after a compile, etc).

peregrineshahin · 2023-09-16T16:01:08Z

TBH it should be done here

                    if (capture || givesCheck) {
                        // Futility pruning for captures (~2 Elo)
                        if (!givesCheck && lmrDepth < 7 && !ss->inCheck &&
                            ss->staticEval + 197 + 248 * lmrDepth +
                                PieceValue[pos.piece_on(to_sq(move))] +
                                captureHistory[movedPiece][to_sq(move)]
                                              [type_of(pos.piece_on(to_sq(move)))] /
                                  7 <
                              alpha)
                            continue;

IMO if the format could be avoided on very long conditions in search.cpp that would be great!
TBH one can already extract rules from the current formatting of Stockfish if that 'automatic' part from the 'automatic formatter' is the goal.

besides that I would assume that doing this inside search.cpp is the way to go then (I didn't know it could be disabled) with this:

// clang-format off
...
// clang-format on

Sopel97 · 2023-09-16T16:28:34Z

Looks fine, certainly better than no automatic formatting.

However, I'd suggest the following additions that should make it closer to the current formatting:

https://clang.llvm.org/docs/ClangFormatStyleOptions.html#allowallparametersofdeclarationonnextline

BinPackParameters: false
AllowAllParametersOfDeclarationOnNextLine: true

https://clang.llvm.org/docs/ClangFormatStyleOptions.html#breakbeforebinaryoperators (should help with the case above)

BreakBeforeBinaryOperators: NonAssignment

https://clang.llvm.org/docs/ClangFormatStyleOptions.html#breakbeforeternaryoperators

BreakBeforeTernaryOperators: true

https://clang.llvm.org/docs/ClangFormatStyleOptions.html#namespaceindentation

NamespaceIndentation: None

https://clang.llvm.org/docs/ClangFormatStyleOptions.html#packconstructorinitializers

PackConstructorInitializers: Never
BreakConstructorInitializers: AfterColon

I also suggest increasing the column limit to 120. It may help with some heavily nested stuff that we do need.

Disservin · 2023-09-17T10:04:51Z

I've updated it with @Sopel97 suggestions and @vondele's awk command

Disservin · 2023-09-17T10:52:01Z

- AllowShortIfStatementsOnASingleLine: WithoutElse
+ AllowShortIfStatementsOnASingleLine: false

vondele · 2023-09-17T10:55:21Z

LGTM

snicolet · 2023-09-17T11:09:00Z

13:08:08 > make format                   
awk '{for(i=1;i<=NF;i++) print $i}' .depend | grep -v '[\\:]$' | xargs realpath | sort | uniq | grep -v incbin | xargs clang-format -i -style=file
xargs: realpath: No such file or directory

snicolet · 2023-09-17T11:09:49Z

Sorry, but this pull request breaks the code readability in so many ways that I consider it a huge regression

Sopel97

while it's pretty good overall, it needs to be less agressive. Especially in places like search. 99% is fine, but the 1% is way too annoying.

Sopel97 · 2023-09-17T10:53:09Z

src/bitboard.cpp


    // Optimal PRNG seeds to pick the correct magics in the shortest time
-    int seeds[][RANK_NB] = { { 8977, 44560, 54343, 38998,  5731, 95205, 104912, 17020 },
-                             {  728, 10316, 55013, 32803, 12281, 15100,  16645,   255 } };
+    int seeds[][RANK_NB] = {{8977, 44560, 54343, 38998, 5731, 95205, 104912, 17020},


we lost spaces before/after braces in initlializer lists

src/nnue/features/half_ka_v2_hm.h

src/nnue/layers/clipped_relu.h

src/position.cpp

Sopel97 · 2023-09-17T11:09:41Z

src/search.cpp

-        && ttValue >= probCutBeta
-        && abs(ttValue) <= VALUE_KNOWN_WIN
-        && abs(beta) <= VALUE_KNOWN_WIN)
+    if (ss->inCheck && !PvNode && ttCapture && (tte->bound() & BOUND_LOWER) && tte->depth() >= depth - 4


yeaaaa I think do need to somehow force old formatting for these, this will get pretty bad quickly

src/thread.cpp

src/types.h

vondele · 2023-09-17T11:22:50Z

13:08:08 > make format                   
awk '{for(i=1;i<=NF;i++) print $i}' .depend | grep -v '[\\:]$' | xargs realpath | sort | uniq | grep -v incbin | xargs clang-format -i -style=file
xargs: realpath: No such file or directory

on mac you might need to install coreutils which could probably be done with https://formulae.brew.sh/formula/coreutils

.clang-format

src/Makefile

.clang-format

peregrineshahin · 2023-09-17T18:15:08Z

We can have breaking before braces specifically for control statements with the Custom option, This was prefered by (Snicolet, Viz, and cj5). for example:

-     BreakBeforeBraces: Attach
+    BreakBeforeBraces: Custom
+    BraceWrapping:
+      AfterFunction: false 
+      AfterClass: true
+      AfterControlStatement: true

snicolet · 2023-09-18T14:26:44Z

I think we should try to keep some of array initializers and conditional alignments (help appreciated) and try to get @snicolet on board

So trying to be constructive:

Concerning the list of .h files, let us use the pedestrian approach of listing the twenty of them in the Makefile explicitely instead of relying of another non-portable tool
Concerning the braces, much better to keep the symetric current style after control statements and else in separate lines, will make the complicated structure of our search mure easier to follow
Concerning the commutative operators appearing on multiple consecutive lines (mainly + and |), I think the problem can be solved by using consecutive assigments, like the rewriting I did when merging Apply penalties for non-threatend pieces moving to threatend squares in mp #4711 (compare the original patch 991c04b and the merged commit 65ece7d)

That is, we can use the following style of coding:

 x = 0;
 x += A;
 x += B;
 x += C;
 x += D;

Concerning the array initialisations, there may be ways to write a clean concat() function to create a large array line by line.
Concerning the non-commutative conditionals (&&, || and ?), I think we have a problem.
For instance, look at the mess that clang-format does for the ternaries operators in movepicker.cpp in the MovePicker::score() function.

locutus2 · 2023-09-20T11:06:12Z

I have tried out the branch and in principal the formatting seems good enough.

But i have two points:

for the MovePicker states enumeration in movepick.cpp all single values are now at a own line. That is unfortunate because each line of states in the current code summarize the states of the different move pickers (main search, evasion, probcut search, quience search) and i would preserve this logic structure for understanding. But perhaps this can be substitutated with a short comment before the first state of each movepicker for clarity.
i have installed clang-format 18.0 and if i run make format i have a little bit different formating as commited in this branch. So is code in this PR already up to date or have we some version dependency?

EDIT:
Here is the diff of this PR and using clang-format at my system:
locutus2/Stockfish@add-clang-format...locutus2:Stockfish:add-clang-format2

Most stuff is more or less idention. Some are good like no big space before one-line comment after statement, but at some points it seems to indent consecutive statements not so that their alligned.

locutus2 · 2023-10-21T17:31:21Z

@Disservin
I have seen you have updated the branch. I have fetched the changes and fired 'make format' and this time i get the same formatting as in your branch, great! I use'Ubuntu clang-format version 18.0.0 (++20231012091728+2ceabf6bdc64-1~~exp1~~20231012091833.1559)'

vondele · 2023-10-21T17:45:50Z

so are you using a development branch of clang?

The newest release is https://github.com/llvm/llvm-project/releases 17.0.3

locutus2 · 2023-10-21T17:47:14Z

@Disservin
Also as i already said i would add some comment to the move picker stages to maintain clear information which stages groups together. I have here locutus2/Stockfish@add-clang-format...locutus2:Stockfish:add-clang-format3 as a proposal which is let unchanged by clang-format.

locutus2 · 2023-10-21T17:53:27Z

@vondele
Until this branch i hadn't used clang-format so i installed directly the newest bleeding edge version! But it seems the 17 version delivers the same formatting after @Disservin updated. Before we had a difference, but which earlier version was used by him i doesn't know.

Disservin · 2023-10-21T17:56:05Z

@vondele Until this branch i hadn't used clang-format so i installed directly the newest bleeding edge version! But it seems the 17 version delivers the same formatting after @Disservin updated. Before we had a difference, but which earlier version was used by him i doesn't know.

I was on 16.0.6, the diff should have only been small though, some macro fixes from clang and some fixed comments. The make target was also assuming that the clang format file is in the parent/root directory, if you instead had one in your local dir you would end up with different formatting, though I dont think that was the case for you ;)

vondele · 2023-10-21T20:27:58Z

I think this is getting in shape for merging.

Most comments that could be integrated in clang-format have been integrated, and I believe the resulting code is pretty easy to read in almost all cases.

The CI integration will also help to keep the format consistent in the future.

Disservin · 2023-10-21T20:29:36Z

The CI integration will also help to keep the format consistent in the future.

as it can be seen here Disservin#20

ddobbelaere · 2023-10-22T06:38:05Z

Nice! Is there a reason why the bot message recommends 17.0.3 (and not the more generic 17)? The action uses a Ubuntu Docker image with clang-format-17 installed. From the logs, I cannot deduce which exact version, but Ubuntu 23.10 is still at 17.0.2 anyway: https://packages.ubuntu.com/mantic/clang-format-17

vondele · 2023-10-22T07:09:43Z

In principle the minor version numbers (17.0.2 and 17.0.3 should give the same format), and I expect any release from the 17 series to give the same result, except when regressions are fixed (i.e. 16.0.2 and 16.0.3 give slightly different results).

It just happens that 17.0.3 is the last released version, and will be what one downloads either here: https://github.com/llvm/llvm-project/releases or it is the one that is obtained if the repo is added in ubuntu, for example for ubuntu 22, see
https://apt.llvm.org/

sudo add-apt-repository 'deb http://apt.llvm.org/jammy/ llvm-toolchain-jammy-17 main'
sudo apt update
sudo apt install clang-format-17

The make file quite easily allow to use the proper version available after unpacking one of the release tarbals, without changing the default compiler on the system, for example:

make format CLANG-FORMAT=/home/vondele/chess/vondele/llvm/clang+llvm-16.0.0-x86_64-linux-gnu-ubuntu-18.04/bin/clang-format

ddobbelaere · 2023-10-22T08:18:39Z

Ok, fine. It just seems overly specific too me to "require/recommend" developers to use 17.0.3. As you mentioned, clang-format seems to follow semantic versioning, so everything with major version 17 should be fine and "not break things" (i.e. result in different formatting).

Just anticipating a potential 17.0.4 or 17.1.0 release, when the message would become outdated/pedantic.

vondele · 2023-10-22T08:28:03Z

Maybe the bot could automatically extract (clang-format --version) its version. I think the precise version should be provided somewhere as part of the message, but maybe not in the way it is formulated right now.

This introduces clang-format to enforce a consistent code style for Stockfish. The lack of an automated code formatter has been a long standing issue official-stockfish#3608. This PR includes a Makefile target to format the code accordingly and write a comment on the PR on GitHub if any formatting changes are required. closes official-stockfish#3608 Co-Authored-By: Joost VandeVondele <[email protected]>

Add a `.git-blame-ignore-revs` file which can be used to skip specified commits when blaming, this is useful to ignore formatting commits, like clang-format #4790. Github blame automatically supports this file format, as well as other third party tools. Git itself needs to be told about the file name to work, the following command will add it to the current git repo. `git config blame.ignoreRevsFile .git-blame-ignore-revs`, alternatively one has to specify it with every blame. `git blame --ignore-revs-file .git-blame-ignore-revs search.cpp` Supported since git 2.23. closes #4969 No functional change

Add a `.git-blame-ignore-revs` file which can be used to skip specified commits when blaming, this is useful to ignore formatting commits, like clang-format official-stockfish#4790. Github blame automatically supports this file format, as well as other third party tools. Git itself needs to be told about the file name to work, the following command will add it to the current git repo. `git config blame.ignoreRevsFile .git-blame-ignore-revs`, alternatively one has to specify it with every blame. `git blame --ignore-revs-file .git-blame-ignore-revs search.cpp` Supported since git 2.23. closes official-stockfish#4969 No functional change

vondele reviewed Sep 16, 2023

View reviewed changes

src/Makefile Outdated Show resolved Hide resolved

Disservin added no-bench-change discussion labels Sep 16, 2023

Disservin force-pushed the add-clang-format branch from d989898 to 2a3a295 Compare September 17, 2023 10:03

Disservin force-pushed the add-clang-format branch 2 times, most recently from d5ce2f6 to 3abc9da Compare September 17, 2023 10:51

Sopel97 suggested changes Sep 17, 2023

View reviewed changes

UniQP reviewed Sep 17, 2023

View reviewed changes

.clang-format Outdated Show resolved Hide resolved

.clang-format Outdated Show resolved Hide resolved

.clang-format Show resolved Hide resolved

src/Makefile Outdated Show resolved Hide resolved

src/Makefile Outdated Show resolved Hide resolved

.clang-format Outdated Show resolved Hide resolved

peregrineshahin reviewed Sep 17, 2023

View reviewed changes

.clang-format Outdated Show resolved Hide resolved

Disservin marked this pull request as draft September 17, 2023 21:07

Disservin added the feature/functionality label Sep 24, 2023

Disservin force-pushed the add-clang-format branch from 3abc9da to 3237177 Compare October 21, 2023 10:00

Disservin force-pushed the add-clang-format branch 4 times, most recently from cb8e2af to c1719da Compare October 22, 2023 12:25

Disservin marked this pull request as ready for review October 22, 2023 12:27

Disservin force-pushed the add-clang-format branch from c1719da to 920e16f Compare October 22, 2023 12:31

vondele added the to be merged Will be merged shortly label Oct 22, 2023

vondele closed this in 2d0237d Oct 22, 2023

Disservin mentioned this pull request Jan 6, 2024

Add .git-blame-ignore-revs #4969

Closed

Disservin mentioned this pull request Sep 2, 2024

Improve build system #5543

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add clang-format #4790

add clang-format #4790

Disservin commented Sep 16, 2023

Disservin commented Sep 16, 2023

peregrineshahin commented Sep 16, 2023 •

edited

Loading

vondele commented Sep 16, 2023

peregrineshahin commented Sep 16, 2023

vondele commented Sep 16, 2023

Disservin commented Sep 16, 2023 •

edited

Loading

vondele commented Sep 16, 2023

peregrineshahin commented Sep 16, 2023

Sopel97 commented Sep 16, 2023 •

edited

Loading

Disservin commented Sep 17, 2023

Disservin commented Sep 17, 2023

vondele commented Sep 17, 2023

snicolet commented Sep 17, 2023

snicolet commented Sep 17, 2023

Sopel97 left a comment •

edited

Loading

Sopel97 Sep 17, 2023

Sopel97 Sep 17, 2023

vondele commented Sep 17, 2023 •

edited

Loading

peregrineshahin commented Sep 17, 2023 •

edited

Loading

snicolet commented Sep 18, 2023 •

edited

Loading

locutus2 commented Sep 20, 2023 •

edited

Loading

locutus2 commented Oct 21, 2023

vondele commented Oct 21, 2023 •

edited

Loading

locutus2 commented Oct 21, 2023 •

edited

Loading

locutus2 commented Oct 21, 2023

Disservin commented Oct 21, 2023

vondele commented Oct 21, 2023

Disservin commented Oct 21, 2023

ddobbelaere commented Oct 22, 2023 •

edited

Loading

vondele commented Oct 22, 2023

ddobbelaere commented Oct 22, 2023 •

edited

Loading

vondele commented Oct 22, 2023

add clang-format #4790

add clang-format #4790

Conversation

Disservin commented Sep 16, 2023

Disservin commented Sep 16, 2023

peregrineshahin commented Sep 16, 2023 • edited Loading

vondele commented Sep 16, 2023

peregrineshahin commented Sep 16, 2023

vondele commented Sep 16, 2023

Disservin commented Sep 16, 2023 • edited Loading

vondele commented Sep 16, 2023

peregrineshahin commented Sep 16, 2023

Sopel97 commented Sep 16, 2023 • edited Loading

Disservin commented Sep 17, 2023

Disservin commented Sep 17, 2023

vondele commented Sep 17, 2023

snicolet commented Sep 17, 2023

snicolet commented Sep 17, 2023

Sopel97 left a comment • edited Loading

Choose a reason for hiding this comment

Sopel97 Sep 17, 2023

Choose a reason for hiding this comment

Sopel97 Sep 17, 2023

Choose a reason for hiding this comment

vondele commented Sep 17, 2023 • edited Loading

peregrineshahin commented Sep 17, 2023 • edited Loading

snicolet commented Sep 18, 2023 • edited Loading

locutus2 commented Sep 20, 2023 • edited Loading

locutus2 commented Oct 21, 2023

vondele commented Oct 21, 2023 • edited Loading

locutus2 commented Oct 21, 2023 • edited Loading

locutus2 commented Oct 21, 2023

Disservin commented Oct 21, 2023

vondele commented Oct 21, 2023

Disservin commented Oct 21, 2023

ddobbelaere commented Oct 22, 2023 • edited Loading

vondele commented Oct 22, 2023

ddobbelaere commented Oct 22, 2023 • edited Loading

vondele commented Oct 22, 2023

peregrineshahin commented Sep 16, 2023 •

edited

Loading

Disservin commented Sep 16, 2023 •

edited

Loading

Sopel97 commented Sep 16, 2023 •

edited

Loading

Sopel97 left a comment •

edited

Loading

vondele commented Sep 17, 2023 •

edited

Loading

peregrineshahin commented Sep 17, 2023 •

edited

Loading

snicolet commented Sep 18, 2023 •

edited

Loading

locutus2 commented Sep 20, 2023 •

edited

Loading

vondele commented Oct 21, 2023 •

edited

Loading

locutus2 commented Oct 21, 2023 •

edited

Loading

ddobbelaere commented Oct 22, 2023 •

edited

Loading

ddobbelaere commented Oct 22, 2023 •

edited

Loading