Skip to main content
7 of 10
added 164 characters in body
chux
  • 36.4k
  • 2
  • 43
  • 97

Doc review

It is possible code takes some of these issues into account.

What if rook has not moved but is not there

With "it only means that neither the king nor the rook corresponding to the castling side have moved throughout the game", a rook should be considered moved if it is taken, even if from its original square. Else one could castle with the ghost rook since it never moved.

50 move rule

State lacks info indicating the last time a piece taken or pawn advanced.

3 repetitions

State lacks info needed for check of the possible outcomes repeating 3 times.

For this and the pervious 50-move concern, a collection of prior and current FEN lines would be enough to record the state. One FEN is not enough to house the state.

Castling

Needs 2 more:

The king is not in check before castling.

The king will not move across check.

Stalemate and others

What if the side lacks a legal move?
What is the FEN state is invalid due to position? (castling flag OK yet no rook, en pass with no pawn, white to move yet no piece.)
How much error checking is desired?


Code review

Good use of const

Good use of pop prefix

Could de more. pop.h creates many named items like NORTH, FILE_A, A1, Square, Color, etc. that may conflict with other code. Consider a more universal use of pop prefix.

Good use of sizeof

Use standard names

u8, u64 may be useful to quickly code, yet lack the clarity of uint8_t, uint64_t. Use the standard.

Avoid assuming unsigned long long and uint64_t are the same

unsigned long long is common today as uint64_t yet is specified to be at least 64-bit. Rather than make unsigned long long constants with #define U64(n) n##ull, use the standard UINT64_C(value) from <stdint.h>.

Allocate to the referenced object, not type

Consider the 2 below. Which do you find easier to code, review and maintain? Which one requires coder to correctly sync type and object?

pos->irreversible = malloc(sizeof(struct irreversible_state));
// vs.
pos->irreversible = malloc(sizeof pos->irreversible[0]);

Magic numbers

Why 6?

for (size_t i = 0; i < 6; ++i)
  pos->type_bb[i] = 0;

Instead:

size_t n = sizeof type_bb / sizeof type_bb[0];
for (size_t i = 0; i < n; ++i)
  pos->type_bb[i] = 0;

Or simply

memset(pos->type_bb, sizeof pos->type_bb, 0);

Style: Why hex?

Surprising use of 0x.

// return piece & 0x1;
return piece & 1;

Style: ()

Even after years of C, code like pt << 1 | c cause me to pause and review those precedence rules. Consider (pt << 1) | c for code that otherwise relies on the less common precedence rules.

Style: {}

Even for 1 line blocks, consider {}.

    //if (!ret)
    //    return 0;
    if (!ret) {
        return 0;
    }

Documentation

The various static functions lack documentation.

Pedantic: is...(negative)

if (isdigit(ch)) is UB when ch < 0 (and not EOF). Use unsigned char ch.

enum size

Note that Piece board[64]; may be wider than 64-bytes as enum piece, which only needs 8-bits, may be wider. Use uint8_t board[64] if reduced size is important.

short?

Little reason for short

 // short fullmove_counter
 int fullmove_counter
chux
  • 36.4k
  • 2
  • 43
  • 97