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

Small cleanup April 2022 #3973

Closed

Conversation

G-Lorenz
Copy link
Contributor

@G-Lorenz G-Lorenz commented Apr 1, 2022

The purpose of this pr is to collect various small cleanups: feel free to leave a comment proposing a new cleanup or a different approach to an existing one!

Commit messages are self-explanatory (at least for the first two cleanups... :-) )

G-Lorenz added 2 commits April 1, 2022 18:33
Compiling with gcc and:
`make build -j ARCH=x86-64-bmi2 debug=yes sanitize=undefined`
shows a warning:
`evaluate.cpp:1115:38: warning: ‘v’ may be used uninitialized
[-Wmaybe-uninitialized]`
The code seems to be safe (i.e. is impossible to evade both ifs) but
the warning is annoying.
This patch somehow mute the warning in `gcc`. I don't know how other
compilers behave.

No functional change
Use `true` instead.
Thanks amchess

No functional change
@locutus2
Copy link
Member

Correct indention for function from_to in types.h line 453 (one space is missing):

thanks locutus2

No functional change
@niklasf
Copy link
Contributor

niklasf commented Apr 23, 2022

// MapKK[] encodes all the 461 possible legal positions of two kings where

There are actually 462 combinations. Later, correctly:

// Usually the first step is to take the wK and bK together. There are just
// 462 ways legal and not-mirrored ways to place the wK and bK on the board.

…2022

(Solve conflicts in evaluate.cpp)

Bench: 7729968
thanks niklasf

No functional change
src/evaluate.cpp Outdated
Comment on lines 1086 to 1092
bool useClassical = (pos.this_thread()->depth > 9 || pos.count<ALL_PIECES>() > 7) &&
abs(eg_value(pos.psq_score())) * 5 > (856 + pos.non_pawn_material() / 64) * (10 + pos.rule50_count());

// Deciding between classical and NNUE eval (~10 Elo): for high PSQ imbalance we use classical,
// but we switch to NNUE during long shuffling or with high material on the board.
if ( !useNNUE
|| ((pos.this_thread()->depth > 9 || pos.count<ALL_PIECES>() > 7) &&
abs(eg_value(pos.psq_score())) * 5 > (856 + pos.non_pawn_material() / 64) * (10 + pos.rule50_count())))
|| useClassical)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this cleanup, makes code easier to read, but the comments describing the code should move with the code.

@vondele
Copy link
Member

vondele commented May 14, 2022

Small CI leftover:

diff --git a/README.md b/README.md
index 6e6e762e6..bc2b1b185 100644
--- a/README.md
+++ b/README.md
@@ -1,7 +1,6 @@
 ## Overview
 
 [![Build Status](https://github.com/official-stockfish/Stockfish/actions/workflows/stockfish.yml/badge.svg)](https://github.com/official-stockfish/Stockfish/actions)
-[![Build Status](https://ci.appveyor.com/api/projects/status/github/official-stockfish/Stockfish?branch=master&svg=true)](https://ci.appveyor.com/project/mcostalba/stockfish/branch/master)
 
 [Stockfish](https://stockfishchess.org) is a free, powerful UCI chess engine
 derived from Glaurung 2.1. Stockfish is not a complete chess program and requires a

@dav1312
Copy link
Contributor

dav1312 commented May 15, 2022

I'm not sure if this could be added to this PR but there is some inconsistent indentation in the nnue_feature_transformer.h and specially in the Makefile

const int8x16_t compacted = vcombine_s8(shifta,shiftb);
return *reinterpret_cast<const vec_t*> (&compacted);

constexpr IndexType OutputChunkSize = MaxChunkSize;

Stockfish/src/Makefile

Lines 825 to 829 in 22b7909

$(eval curl_or_wget := $(shell if hash curl 2>/dev/null; then echo "curl -skL"; elif hash wget 2>/dev/null; then echo "wget -qO-"; fi))
@if test -f "$(nnuenet)"; then \
echo "Already available."; \
else \
if [ "x$(curl_or_wget)" = "x" ]; then \

Also, it looks like the last commit added a seemingly unnecessary new line

Stockfish/src/evaluate.cpp

Lines 193 to 195 in 22b7909

namespace {

G-Lorenz and others added 4 commits May 17, 2022 09:11
Thanks vondele

No functional change
remove extra line in evaluate.cpp
fix indentation of inlines in `nnue_feature_trasformer.h`
fix indentation of ifdefs in `Makefile` (using tabs)

thanks dav1312

No functional change
@Disservin
Copy link
Member

futility_move_count has its own function and is only used in one place, thus this can simply be removed
similar to this master...Disservin:removeFunc
maybe even leaving the int futility_move_count away

@pb00067
Copy link

pb00067 commented May 18, 2022

For me the logic in probcut's TT condition is hard to follow:

             && !(   ss->ttHit
             && tte->depth() >= depth - 3
             && ttValue != VALUE_NONE
             && ttValue < probCutBeta))

Personally I would prefer this equivalent which corresponds more to human thinking

             && (   !ss->ttHit
                   || tte->depth() < depth - 3
                   || ttValue == VALUE_NONE
                   || ttValue >= probCutBeta))

@dubslow
Copy link
Contributor

dubslow commented May 18, 2022

For me the logic in probcut's TT condition is hard to follow:

In either case, a grouped-together set of conditions should get more indentation for the operators within that group

@dsmsgms
Copy link
Contributor

dsmsgms commented May 28, 2022

Stack->depth is simply not used. Might as well remove the field from the struct.

@niklasf
Copy link
Contributor

niklasf commented May 28, 2022

README.md refers to Readme.md (inconsistent casing)

@vondele
Copy link
Member

vondele commented May 29, 2022

I'll be merging this shortly, thanks for maintaining this PR.

@vondele vondele added the to be merged Will be merged shortly label May 29, 2022
@vondele vondele closed this in f7d1491 May 29, 2022
@mstembera
Copy link
Contributor

The comment in evaluate.cpp starting with "// Deciding between..." is now duplicated on lines 1086 and 1091 after this PR.

@vondele
Copy link
Member

vondele commented May 30, 2022

that was a merge mistake... will fix. Probably a good reason to start a new cleanup PR ;-)

dav1312 pushed a commit to dav1312/Stockfish that referenced this pull request Oct 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
to be merged Will be merged shortly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants