Investigate if this code change makes sense

[hgducharme]

We have the following:

Bitboard calculatePawnAttacksFromSquare(const Color color, const Bitboard & bitboard)
{
    Bitboard potentialPawnAttacks;
    Rank rank = Chessboard::squareToRank(bitboard.findIndexLSB());

    // A pawn only has valid moves between ranks 2 and 7
    if (rank == RANK_1 || rank == RANK_8)
    {
        return potentialPawnAttacks;
    }

    // White pawns can only move north and black pawns can only move south
    Direction eastDirection = NORTH_EAST;
    Direction westDirection = NORTH_WEST;
    if (color == BLACK)
    {
        eastDirection = SOUTH_EAST;
        westDirection = SOUTH_WEST;
    }

    // If we perform a move and end up on the opposite side of the board, that is an off-board move and we need to exclude that move
    potentialPawnAttacks |= utils::shiftCurrentSquareByDirection(bitboard, eastDirection) & constants::bit_masks::EXCLUDE_FILE_A;
    potentialPawnAttacks |= utils::shiftCurrentSquareByDirection(bitboard, westDirection) & constants::bit_masks::EXCLUDE_FILE_H;

    return potentialPawnAttacks;
}

But the inside code doesn't even need bitboard. It just needs the square as an integer value. Maybe we can do this:

Bitboard calculatePawnAttacksFromSquare(const Color color, const Square square)
{
    Bitboard potentialPawnAttacks;
    Rank rank = Chessboard::squareToRank(square);

    ...
}

There are many such changes like this to be made in the file attacktables.cpp

[hgducharme]

I do the same thing here:

Bitboard shiftCurrentSquareByDirection(const Square square, const int numberOfBits)
{
   Bitboard squareAsBitboard(square);
   return shiftCurrentSquareByDirection(squareAsBitboard, numberOfBits);
}

Bitboard shiftCurrentSquareByDirection(const Bitboard & oldPosition, const int numberOfBits)
{
   Bitboard newPosition;

   if (numberOfBits > 0)
   {
      newPosition = oldPosition << numberOfBits;
   }
   else
   {
      newPosition = oldPosition >> std::abs(numberOfBits);
   }

   return newPosition;
}

Note that the first example in the CPP Core Guidelines explicitly says not to do this.

Also on the topic of the method shiftSquareByDirection(), it seems like I only use this function in order to get a bitboard representation of a particular square and then use this resulting bitboard as a bitmask with another bitboard. Why am I doing this wen I already have the function Bitboard.getNeighbor()? Although this function name kind of implies the neighbor is touching the bitboard. We would need to find out a good way to reconcile the semantics of all of this.

Ultimately I am interested in the shiftSquareByDirection() method because I have a square and I want to move away from the current square by some vector, and then I need a bitboard representation of the landing square.

Things to also consider:

• If we return a square maybe all we need is to override some of the bitwise operators for a square, so that we can do legalMoves |= square. It would look great wherever it's used, instead of always having to convert to and from a bitboard representation of a square, etc.

[hgducharme]

Ok so I implemented the following change:

// utils.h
Bitboard shiftSquareByDirection(const Square square, const int numberOfBits);
Bitboard shiftSquareByDirection(const Bitboard & oldPosition, const int numberOfBits);

became:

// utils.h
Bitboard getSquareInDirectionAsBitboard(const Square square, const int numberOfBits);

That is, I changed the name of the function to be more precise (per #42) and deleted the overload that took a Bitboard argument. Now all calls to this function pass in a Square, and it alleviates the concerns of this issue.

[hgducharme]

Closed by b5d3b0e in #56