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

Collective Cleanups February 2021 #3329

Conversation

Lolligerhans
Copy link
Contributor

@Lolligerhans Lolligerhans commented Jan 30, 2021

Merge/pull with --squash to prepare single commit in staging index, commit with git commit.
Created empty following request from #3301.


What is a cleanup?

(Trivial) cleanups are changes which do not require testing, because their benefit is apparent to everyone. This can include

  • Spelling and orthography corrections
  • Code style harmonization/homogenization
  • Commentary refinement
  • (Other small changes of evident benefit)

Participation

You have two options to add your own cleanups:

  1. Create a separate PR on my branch trivial-cleanups_Feb2021 to my fork of Stockfish. This allows me to to easily merge your ideas, if they require large or complex changes.
  2. For simple changes, comment below and I add them for you.

Legend

👀 = I have seen your comment
👍 = Comment is resolved or discussed further below

@BM123499
Copy link
Contributor

Missing constexpr on score in movepick.cpp

Template member fcuntion cannot be used outside movepicker.cpp anyway.

No functional change
@BM123499
Copy link
Contributor

@Lolligerhans, I meant inside... ;)

@Lolligerhans
Copy link
Contributor Author

@BM123499 I saw you test now and figured! xD The function one is on the house. 👍

Copy link
Contributor

@BM123499 BM123499 left a comment

Choose a reason for hiding this comment

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

we are using constexpr in many places, but can they be used on line 428 of evaluate.cpp?

@Lolligerhans
Copy link
Contributor Author

we are using constexpr in many places, but can they be used on line 428 of evaluate.cpp?

@BM123499 Please add a commit with a concrete suggestion. You will need to address the defined-not-used warning resulting from excluding all uses of some of the constexpr variables from the function body. I originally just omitted this case for simplicity (keeping all constexpr definitions in a block at start), but surely there are technical solutions for this problem.

@snicolet
Copy link
Member

Line 271-272 of position.cpp should be formatted like this:

               && (   file_of(square<KING>(sideToMove)) == file_of(st->epSquare)
                   || !(blockers_for_king(sideToMove) & (st->epSquare + pawn_push(~sideToMove))));

@snicolet
Copy link
Member

snicolet commented Feb 11, 2021

Lines 521-522 of position.cpp should be like this:

      return    !(st->previous->blockersForKing[sideToMove] & from)
             || aligned(from, to, square<KING>(us));

EDIT: removed the faulty de Morgan law, sorry. Corrected to just push the ||on second line.

@snicolet
Copy link
Member

Lines 660-661 of position.cpp should be formatted like this:

      return   st->previous->checkersBB 
            || (   rank_of(square<KING>(~sideToMove)) == rank_of(from)
                && (st->previous->blockersForKing[~sideToMove] & from));

@BM123499
Copy link
Contributor

Lines 521-522 of position.cpp should be like this:

      return    !(st->previous->blockersForKing[sideToMove] & from)
             && !aligned(from, to, square<KING>(us));

@snicolet it changes the logic.

- move boolen operators that span larger compaound expressions to the
  beginning of a line
- indent sub-expressions fully to the right of the encompassing operator
@Lolligerhans
Copy link
Contributor Author

Adjusted whitespace to follow suggested indentation. Kept original expressions where suggested corrections differ (as pointed out by BM123499). I suppose these suggestions were accidental (?)

@snicolet
Copy link
Member

snicolet commented Feb 11, 2021

Yes, sorry :-)
Corrected to just push the || operator on the second line in lines 521-522.

@vondele
Copy link
Member

vondele commented Feb 12, 2021

fix copyright date in Makefile

@Lolligerhans
Copy link
Contributor Author

fix copyright date in Makefile

@vondele
(1) Adding for time until today or

 # Copyright (C) 2004-2008 Tord Romstad (Glaurung author)
 # Copyright (C) 2008-2015 Marco Costalba, Joona Kiiski, Tord Romstad
 # Copyright (C) 2015-2019 Marco Costalba, Joona Kiiski, Gary Linscott, Tord Romstad
+# Copyright (C) 2019-2021 The Stockfish developers (see AUTHORS file)
 #
 # Stockfish is free software: you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by

(2) replacing entire time span?

 # Stockfish, a UCI chess playing engine derived from Glaurung 2.1
-# Copyright (C) 2004-2008 Tord Romstad (Glaurung author)
-# Copyright (C) 2008-2015 Marco Costalba, Joona Kiiski, Tord Romstad
-# Copyright (C) 2015-2019 Marco Costalba, Joona Kiiski, Gary Linscott, Tord Romstad
+# Copyright (C) 2004-2021 The Stockfish developers (see AUTHORS file)
 #
 # Stockfish is free software: you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by

@snicolet
Copy link
Member

I would go for option (2) replacing entire time span, as in other source files.

@snicolet
Copy link
Member

for search.cpp:
• lines 1030-1034 (in the likelyFailLow block): trailing spaces at end of lines
• line 1083: trailing space
• line 1084: end of two lines comment is missing a point.

@snicolet
Copy link
Member

Merged via 40cb0f0, thanks :-)

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

4 participants