Make Square and Bitboard operators commutative

As Stockfish developers, we aim to make our code as legible and as close
to simple English as possible. However, one of the more notable exceptions
to this rule concerns operations between Squares and Bitboards.

Prior to this pull request, AND, OR, and XOR were only defined when the
Bitboard was the first operand, and the Square the second. For example,
for a Bitboard b and Square s, "b & s" would be valid but "s & b" would not.
This conflicts with natural reasoning about logical operators, both
mathematically and intuitively, which says that logical operators should
commute.

More dangerously, however, both Square and Bitboard are defined as integers
"under the hood." As a result, code like "s & b" would still compile and give
reasonable bench values. This trap occasionally ensnares even experienced
Stockfish developers, but it is especially dangerous for new developers not
aware of this peculiarity. Because there is no compilation or runtime error,
and a reasonable bench, only a close review by approvers can spot this error
when a test has been submitted--and many times, these bugs have slipped past
review. This is by far the most common logical error on Fishtest, and has
wasted uncountable STC games over the years.

However, it can be fixed by adding three non-functional lines of code. In this
patch, we define the operators when the operands are provided in the opposite
order, i.e., we make AND, OR, and XOR commutative for Bitboards and Squares.
Because these are inline methods and implemented identically, the executable
does not change at all.

This patch has the small side-effect of requiring Squares to be explicitly
cast to integers before AND, OR, or XOR with integers. This is only performed
twice in Stockfish's source code, and again does not change the executable at
all (since Square is an enum defined as an integer anyway).

For demonstration purposes, this pull request also inverts the order of one AND
and one OR, to show that neither the bench nor the executable change. (This
change can be removed before merging, if preferred.)

I hope that this pull request significantly lowers the barrier-of-entry for new
developer to join the Stockfish project. I also hope that this change will improve
our efficiency in using our generous CPU donors' machines, since it will remove
one of the most common causes of buggy tests.

Following helpful review and comments by Michael Stembera (@mstembera), we add
a further clean-up by implementing OR for two Squares, to anticipate additional
traps developers may encounter and handle them cleanly.

Closes https://github.com/official-stockfish/Stockfish/pull/2387

No functional change.

diff --git a/src/bitbase.cpp b/src/bitbase.cpp
index 2b1a5517fb8152df1809f0f508e082177660fb39..9301dcfad13ea51797b10d44e3011c9b7fa36f90 100644 (file)
--- a/src/bitbase.cpp
+++ b/src/bitbase.cpp
@@ -44,7 +44,7 @@ namespace {
   // bit 13-14: white pawn file (from FILE_A to FILE_D)
   // bit 15-17: white pawn RANK_7 - rank (from RANK_7 - RANK_7 to RANK_7 - RANK_2)
   unsigned index(Color us, Square bksq, Square wksq, Square psq) {
   // bit 13-14: white pawn file (from FILE_A to FILE_D)
   // bit 15-17: white pawn RANK_7 - rank (from RANK_7 - RANK_7 to RANK_7 - RANK_2)
   unsigned index(Color us, Square bksq, Square wksq, Square psq) {

-    return wksq | (bksq << 6) | (us << 12) | (file_of(psq) << 13) | ((RANK_7 - rank_of(psq)) << 15);
+    return int(wksq) | (bksq << 6) | (us << 12) | (file_of(psq) << 13) | ((RANK_7 - rank_of(psq)) << 15);

   }
 
   enum Result {
   }
 
   enum Result {

diff --git a/src/bitboard.h b/src/bitboard.h
index 477b1655d5d5c2ece4317c97531716caddae20ba..8d748eeed8615628ddd4b044df9c34aef4fcaeff 100644 (file)
--- a/src/bitboard.h
+++ b/src/bitboard.h
@@ -119,6 +119,12 @@ inline Bitboard  operator^( Bitboard  b, Square s) { return b ^  square_bb(s); }
 inline Bitboard& operator|=(Bitboard& b, Square s) { return b |= square_bb(s); }
 inline Bitboard& operator^=(Bitboard& b, Square s) { return b ^= square_bb(s); }
 
 inline Bitboard& operator|=(Bitboard& b, Square s) { return b |= square_bb(s); }
 inline Bitboard& operator^=(Bitboard& b, Square s) { return b ^= square_bb(s); }
 

+inline Bitboard  operator&(Square s, Bitboard b) { return b & s; }
+inline Bitboard  operator|(Square s, Bitboard b) { return b | s; }
+inline Bitboard  operator^(Square s, Bitboard b) { return b ^ s; }
+
+inline Bitboard  operator|(Square s, Square s2) { return square_bb(s) | square_bb(s2); }
+

 constexpr bool more_than_one(Bitboard b) {
   return b & (b - 1);
 }
 constexpr bool more_than_one(Bitboard b) {
   return b & (b - 1);
 }

diff --git a/src/endgame.cpp b/src/endgame.cpp
index e10f8d5da975646d236468f6dc251f0052df60bb..ca38a662a01a588d1fa11cf0eba21d2b2b156582 100644 (file)
--- a/src/endgame.cpp
+++ b/src/endgame.cpp
@@ -74,7 +74,7 @@ namespace {
     assert(pos.count<PAWN>(strongSide) == 1);
 
     if (file_of(pos.square<PAWN>(strongSide)) >= FILE_E)
     assert(pos.count<PAWN>(strongSide) == 1);
 
     if (file_of(pos.square<PAWN>(strongSide)) >= FILE_E)

-        sq = Square(sq ^ 7); // Mirror SQ_H1 -> SQ_A1
+        sq = Square(int(sq) ^ 7); // Mirror SQ_H1 -> SQ_A1

 
     return strongSide == WHITE ? sq : ~sq;
   }
 
     return strongSide == WHITE ? sq : ~sq;
   }

diff --git a/src/evaluate.cpp b/src/evaluate.cpp
index ea54e271390e68e5a0f65cbb63537628ef0fd6d9..cefd9db427f19fd43f1d51ee08d621bec0b4cd61 100644 (file)
--- a/src/evaluate.cpp
+++ b/src/evaluate.cpp
@@ -237,7 +237,7 @@ namespace {
     // Init our king safety tables
     Square s = make_square(clamp(file_of(ksq), FILE_B, FILE_G),
                            clamp(rank_of(ksq), RANK_2, RANK_7));
     // Init our king safety tables
     Square s = make_square(clamp(file_of(ksq), FILE_B, FILE_G),
                            clamp(rank_of(ksq), RANK_2, RANK_7));

-    kingRing[Us] = PseudoAttacks[KING][s] | s;
+    kingRing[Us] = s | PseudoAttacks[KING][s];

 
     kingAttackersCount[Them] = popcount(kingRing[Us] & pe->pawn_attacks(Them));
     kingAttacksCount[Them] = kingAttackersWeight[Them] = 0;
 
     kingAttackersCount[Them] = popcount(kingRing[Us] & pe->pawn_attacks(Them));
     kingAttacksCount[Them] = kingAttackersWeight[Them] = 0;


@@ -291,7 +291,7 @@ namespace {
         {
             // Bonus if piece is on an outpost square or can reach one
             bb = OutpostRanks & attackedBy[Us][PAWN] & ~pe->pawn_attacks_span(Them);
         {
             // Bonus if piece is on an outpost square or can reach one
             bb = OutpostRanks & attackedBy[Us][PAWN] & ~pe->pawn_attacks_span(Them);

-            if (bb & s)
+            if (s & bb)

                 score += Outpost * (Pt == KNIGHT ? 2 : 1);
 
             else if (Pt == KNIGHT && bb & b & ~pos.pieces(Us))
                 score += Outpost * (Pt == KNIGHT ? 2 : 1);
 
             else if (Pt == KNIGHT && bb & b & ~pos.pieces(Us))

diff --git a/src/position.h b/src/position.h
index e6c901ea2c98793e68ae7a17927a12fdefa65af5..2ec2729cf3a256efcdeaa53a2fc0ca5a725a20ee 100644 (file)
--- a/src/position.h
+++ b/src/position.h
@@ -430,7 +430,7 @@ inline void Position::move_piece(Piece pc, Square from, Square to) {
 
   // index[from] is not updated and becomes stale. This works as long as index[]
   // is accessed just by known occupied squares.
 
   // index[from] is not updated and becomes stale. This works as long as index[]
   // is accessed just by known occupied squares.

-  Bitboard fromTo = square_bb(from) | square_bb(to);
+  Bitboard fromTo = from | to;

   byTypeBB[ALL_PIECES] ^= fromTo;
   byTypeBB[type_of(pc)] ^= fromTo;
   byColorBB[color_of(pc)] ^= fromTo;
   byTypeBB[ALL_PIECES] ^= fromTo;
   byTypeBB[type_of(pc)] ^= fromTo;
   byColorBB[color_of(pc)] ^= fromTo;