RENAMING/REFORMATTING THREAD

[mcostalba]

Use this issue to post renaming suggestions, once in a while, if approved will be committed at once.

We will use a table to make it easier account pending renames.

Please add your renames as new rows in the table. Thanks.

[mcostalba]

Proposed pending renames:

Current name	Proposed rename	Rationale
shift(Bitboard b)	shift_bb(Bitboard b)	Consistency: all functions returning a Bitboard are _bb suffixed

[snicolet]

Good idea to open this issue for renaming, thanks!

I was actually thinking of opening some other permanent thread like that to accumulate the SMALL REFORMATS for a period, and test the accumulated set at the end of each period (say one month) as a normal double [-3..1] test: this would avoid slowing down the framework for zillions of small reformating edits, while still having the assurance that the reformats are not introducing hidden speed regressions.

Re: _bb suffix. My opinion is that the _bb suffix is fine for functions which actually change the type of their parameter, to indicate that the codomain of the function is Bitboard.

But I think we should keep shift(Bitboard b), as it is so clean and readable, and is actually generalizing a shift of bitboard that already exists in C (the << operator) that everybody calls "shift".

[snicolet]

@mcostalba Can everybody edit the table? We don't have the credentials to edit comments, it seems...

[mcostalba]

@snicolet hmm indeed people is supposed to copy the table into a new one adding their row....

[mcostalba]

....maybe opening a wiki page, referenced by here could be better....

[jerrydonaldwatson]

Some more suggestions (can't edit table above, so posting here instead):

Current name	Proposed rename	Rationale
kingAdjacentZoneAttacksCount	kingAdjacentAttacksCount	The name is already very long and the word "Zone" does not aid understanding.
LongRangedBishop	LongDiagonalBishop	Slightly more accurate definition in my opinion.
TrappedBishopA1H1	TrappedCornerBishop	Slightly more descriptive name in my opinion.
unstablePvFactor	unstableBestMoveFactor	Slightly more accurate definition in my opinion, after all this does not measure changes to the PV apart from the best move itself.
rootNode	RootNode	Consistency: we already have PvNode.

edited in view of comments below - thanks!

[mcostalba]

@jerrydonaldwatson thanks for your suugestions that I find good. Just to clarify regaridng capitalization convention:

• If it is a variable the name first char is lowercased
• If it is a template parameter or a constant the first char is uppercased

[syzygy1]

I think it should be PvNode and RootNode, because they are defined as constants.

Of course pvExact is just as much a constant, except that its constant value can be determined only halfway the function.

And then there is "const PieceToHistory* contHist[]" which should perhaps be ConstHist[] because of the "const". In the next line, "Move countermove" could become "const Move CounterMove".

If we want to "fix" this, maybe someone should first draw up a guideline for capitalisation (and for const usage).

Btw. I don't think the usage of "const" for local variables improves optimisation. Its only role is to catch programming errors.

[jerrydonaldwatson]

@mcostalba Thanks a lot for that, now I understand that one. @syzygy1 Yes, PvNode and RootNode may be a better choice. I just noticed a small inconsistency which can be remedied in this way.

I've edited the table to reflect these comments.

[mcostalba]

@syzgy1 there is a difference between a constant and a variable that happens to be used as constant in a specific function.

PvNode is a real constant, instead history tables of course are not, they are just not changed in some functions.

[mcostalba]

To be more clear, we can define constant as compile time constant:

• If name refers to a template parameter, a compile time constant, a global variable, a class or struct, a namespace, an enum type then the first char is uppercased

• In all the other cases the name first char is lowercased

[syzygy1]

Then it might make sense to use constexpr for PvNode?

[syzygy1]

Indeed constexpr bool PvNode compiles correct, whereas constexpr bool rootNode does not.

[mcostalba]

Yes, constexpr could be a good code's self-documenting tool in this case.

[gonlem]

Correcting a supposed typo in a method name.

Current name	Proposed rename	Rationale
kingAdjacentZoneAttacksCount	kingAdjacentAttacksCount	The name is already very long and the word "Zone" does not aid understanding.
LongRangedBishop	LongDiagonalBishop	Slightly more accurate definition in my opinion.
TrappedBishopA1H1	TrappedCornerBishop	Slightly more descriptive name in my opinion.
unstablePvFactor	unstableBestMoveFactor	Slightly more accurate definition in my opinion, after all this does not measure changes to the PV apart from the best move itself.
rootNode	RootNode	Consistency: we already have PvNode.
is_KBPsKs(...)	is_KBPsK(...)	This method name is inconsistent with the related ScalingFunction "ScaleKBPsK" (see line 71 & 168 of material.cpp).

[protonspring]

Instead of TrappedCornerBishop. . . Just. . "CorneredBishopA1" etc. Sorry, i cant edit the table and i am on my phone.

[pb00068]

I suggest to rename pinnersForKing into pinnersOnKing because a 'pinner' actually pins a piece on a king, thus 'on' may be more correct than 'for'.

[xoto10]

Current name	Proposed rename	Rationale
kingAdjacentZoneAttacksCount	kingAdjacentAttacksCount	The name is already very long and the word "Zone" does not aid understanding.
LongRangedBishop	LongDiagonalBishop	Slightly more accurate definition in my opinion.
TrappedBishopA1H1	TrappedCornerBishop	Slightly more descriptive name in my opinion.
unstablePvFactor	unstableBestMoveFactor	Slightly more accurate definition in my opinion, after all this does not measure changes to the PV apart from the best move itself.
rootNode	RootNode	Consistency: we already have PvNode.
is_KBPsKs(...)	is_KBPsK(...)	This method name is inconsistent with the related ScalingFunction "ScaleKBPsK" (see line 71 & 168 of material.cpp).
pinnersForKing	pinnersOnKing	position.h/.cpp
v	sc	line 115 in psqt.cpp

[Rocky640]

Why not simply pinnersForKing[WHITE] => pinners[BLACK] (pieces which belongs to black side) pinnersForKing[BLACK] => pinners[WHITE] (pieces which belongs to white side)

[syzygy1]

@Rocky640 Because pinnersForKing[WHITE] contains both white and black pieces.

[Rocky640]

I'm quite sure that pinnersForKing[WHITE] contains only some black pieces. However blockersForKing[WHITE] contains both white and black pieces.

[syzygy1]

You are right, I am wrong :-)

[vondele]

Current name	Proposed rename	Rationale
kingAdjacentZoneAttacksCount	kingAdjacentAttacksCount	The name is already very long and the word "Zone" does not aid understanding.
LongRangedBishop	LongDiagonalBishop	Slightly more accurate definition in my opinion.
TrappedBishopA1H1	TrappedCornerBishop	Slightly more descriptive name in my opinion.
unstablePvFactor	unstableBestMoveFactor	Slightly more accurate definition in my opinion, after all this does not measure changes to the PV apart from the best move itself.
rootNode	RootNode	Consistency: we already have PvNode.
is_KBPsKs(...)	is_KBPsK(...)	This method name is inconsistent with the related ScalingFunction "ScaleKBPsK" (see line 71 & 168 of material.cpp).
pinnersForKing	pinnersOnKing	position.h/.cpp
v	sc	line 115 in psqt.cpp
InCheck	inCheck	in qsearch, needed after turning it from a template param to a local variable

[mcostalba]

@vondele thanks for the update.

Please note that rootNode is correct because is defined as;

const bool rootNode = PvNode && ss->ply == 0;

and is not a compile time constant (instead PvNode it is):

const bool PvNode = NT == PV;

[snicolet]

My two cents:

• I like Alain's suggestion of pinnersForKing -> pinners • InCheck -> inCheck is done • I would do v -> score in line 115 of psqt.cpp • all the other renaming proposals in Joost table are fine with me

[snicolet]

And I would propose contempt -> optimism everywhere.

[snicolet]

Implemented all (most) of the suggestions so far in this commit: 96362fe3df141eeead4bdb863d2bb2d891886abf

[Rocky640]

in timeman.cpp line 93 and 100, fix 2 comments:

// WARNING: Given npms (nodes per millisecond) must be much lower then =>Given npmsec // Convert from millisecs to nodes ==> milliseconds

[snicolet]

In Movepicker.cpp, rename the goto label:

   goto begin_switch   =>  goto top

[syzygy1]

I noticed that my constexpr suggestion was committed. Should we now mark all compile-time local constants as constexpr (and verify that just those start with a capital)?

[vondele]

@syzygy1 I think that is a reasonable thing to do.

[jerrydonaldwatson]

The comment on line 378 of evaluate.cpp: // Bonus for aligning rook with with enemy pawns on the same rank/file

should be amended to remove the double word "with".

[Rocky640]

The comment on line 695 of evaluate.cpp: } // rr != 0

should be } // w != 0

[xoto10]

thread.cpp line 120 "... launched threads will ..."

[Rocky640]

evaluate.cpp line 813

// Endings where weaker side can place his king in front of the enemy's
  // pawns are drawish.

could be replaced with

// Endings where weaker side can place his king in front of at least one enemy
  // pawn are drawish.

[syzygy1]

This comment has been incorrect now for a couple of years: https://github.com/official-stockfish/Stockfish/blob/e408fd7b10503a9114962cb5972abd9957bc67c2/src/types.h#L260-L268 Nowadays, the least significant 16 bits are used to store the middlegame value and the upper 16 bits are used to store the endgame value.

[Rocky640]

In timeman.cpp

  // move_importance() is a skew-logistic function based on naive statistical
  // analysis of "how many games are still undecided after n half-moves". Game
  // is considered "undecided" as long as neither side has >275cp advantage.
  // Data was extracted from the CCRL game database with some simple filtering criteria.

This comment had been there since sf 2.3.1 and possibly before ! In sf 2.3.1, the move importance was encoded in a big array A formula was introduced in sf 5 and the curve looks different. Current values in sf9 are still quite similar to sf5.

I'm not sure how to "fix" that comment, but for sure, it is not accurate anymore.

[Matt14916]

The timeman.cpp move importance comment was originally left in after the fit so the origin of the fit and parameters would be documented, I think. The current values of the parameters are probably outside the original (questionable) error bars, but the history is probably good to mention, updated in some way ("originally based on" perhaps).

Since that area was brought up, in timeman.cpp, XScale and XShift should probably be named PlyScale and PlyShift or something similar. They are characteristic ply values for the function. The current names just reflect how the fitting was done and should have been chosen better originally.

[Rocky640]

@Matt14916 thanks for your explanations. Yes "originally based on..." could help. However, the explanation is not clear anyway, I would not know how to reproduce this original experiment even if I would have the original data (or some new one...) And from now on, all we will have there are tuned values.

There might be something to improve SF strength here. Maybe with the new contempt, the avg game length has changed. Also I wonder if the avg # plies should be different according to time control.

[jerrydonaldwatson]

@Rocky640 I also think there is room for improvement here. Another aspect is that Kai Laskos posted some data on Talkchess ( www.talkchess.com/forum/viewtopic.php?t=66821) which suggested that opening moves were less important than middlegame moves. In line with this, I tried to change the curve to use less time in the opening, and got two yellows:

http://tests.stockfishchess.org/tests/view/5aac657a0ebc5902975929e7 http://tests.stockfishchess.org/tests/view/5aace5ab0ebc590297592a65

[xoto10]

@Rocky640 @jerrydonaldwatson Speaking of time management, one thing I have wondered about is when sf gets down to playing on increment. It seems to me that the remaining time typically gets too low, and then there is no flexibility to use 3x or 4x increment (or more) on a tricky move as it normally would. I'm not sure how to adjust it, but I think there might be a benefit in using very slightly less time when it is running out so that remaining time levels out at say 60s when playing with a 10s increment instead of levelling out at 20s or less.

[protonspring]

This has bit me a few times. EVERYWHERE in the code, BLACK means the side playing the black pieces and WHITE means the side playing the white pieces, except for pawnsOnSquares. It is a bit disorienting to see e->pawnsOnSquares[Us][BLACK] or [WHITE], but here it means the color of squares NOT the color of the pieces.

https://github.com/protonspring/Stockfish/tree/ps_lightdark

[Rocky640]

In bitboard.cpp, line 141, there are unnessary spaces in the following enum Direction RookDirections[] = { NORTH, EAST, SOUTH, WEST };

[Rocky640]

in movepick.h, line 120 some extra spaces

MovePicker(const Position&, Move, Depth, const ButterflyHistory*,  const CapturePieceToHistory*, Square);

[Rocky640]

in evaluate.cpp line 736, the comment had not been updated

    // Find the safe squares for our pieces inside the area defined by
    // SpaceMask. A square is unsafe if it is attacked by an enemy
    // pawn, or if it is undefended and attacked by an enemy piece.
    Bitboard safe =   SpaceMask
                   & ~pos.pieces(Us, PAWN)
                   & ~attackedBy[Them][PAWN];

I suggest // Find the available squares for our pieces inside the area defined by SpaceMask

[Rocky640]

In threads,cpp, line 120 replace wil with will

[snicolet]

pawns.cpp, unnecessary parentheses there:

if ((shift<Down>(theirPawns) & (FileABB | FileHBB) & BlockRanks) & ksq)

[protonspring]

In pawns.cpp, there is a Bitboard called "supported," but it doesn't include pawns that are supported. Instead, it includes the pawns that support. I think "support," or "supporters" better reflects what this Bitboard is doing.

[shooreek]

As it was already spotted by @Rocky640 few comments ago:

In threads,cpp, line 120 replace wil with will

"threads wil go immediately to sleep" -> "threads will immediately go to sleep"

[syzygy1]

In the Thread class, the nmp_ply and nmp_odd members should be renamed nmpPly and nmpOdd.

In the same class, the PVIdx and PVLast members should probably be renamed pvIdx and pvLast. See e.g. the tbHits member.

In that case, the TBRank and TBScore members of the RootMove struct should be renamed tbRank and tbScore.

In line 976 of tbprobe.cpp, comrpession should be compression (simple typo in a comment).