Problem with bitboard knight attack generator

[Rein Halbersma]

From my experience it is simply the wrong view to say that "fixing some error checking is a waste of time". It is always cheaper to include error checking immediately when designing the algorithm. If you add it later then you add much more testing effort, and there is a high risk that you introduce new errors by changing the algorithm (or its implementation) to include error checking. Better include it right from the beginning.

Part of that "error checking" topic can also be to include ASSERT's, which can sometimes help to reduce the amount of error checking code of the release version drastically. (But ASSERT's are not always the right solution, e.g. not always applicable for external input.)

To a certain degree this does also apply to chess programs, where an FEN parser is only one (perhaps less important) example.

Underestimating the need for error checking and/or ASSERT's is one of the best ways to unreliable code.

Sven

I also like the "if you haven't got the time to do it right, where are you going to find the time to do it over?" philosophy. For my draughts program, I have automated unittesting (using Boost.Test, and previously GoogleTest) in place.

All the low-level routines (bit-twiddling, move generation, hash lookup/storage) are automatically checked exhaustively (at least the corner cases) on *each* compile. It's simply a post-build event in Visual Studio, so you cannot ever break code without noticing immediately. The code simply fails to build successfully if any unittest fails! It only takes a few seconds (on a compile time of a minute) to run this. The more time consuming tests (search, perft) are not done on every build, but are automatically scheduled for an overnight build (they tend to take between 2 and 6 hours).

Surprisingly, I never read about anyone at this forum doing unittesting. Is that because people consider this a hobby and don't like the effort, or is it because of lack of familiarity with the concept?

[Dave_N]

David,

stop.

Matthew:out

sorry didn't mean to hijack the thread with a discussion about error checking.

{classic example, MyQuad square; square.x = calculated_number; assert(square.x != -INFINITY);}

[Sven]

Probably the best practice, although in any app there are likely to be non-error checked assignments and variable usage. If for example you have a class called Foo ...

Code: Select all

class Foo
{
public:
   Foo()
{
    mem = new SmallBlockOfMemory(this); // new fails because the //system is out of memory
}
~Foo()
{
if( mem )
{
    delete mem;
   mem = NULL;
}
}

SmallBlockOfMemory *mem; // with own constructor
};

and "new" isn't overridden until much later when Foo has become a major project.

Sorry, I don't quite get the point here. Which "non error-checked assignment" did you want to show with that code snippet?

Btw, you can omit NULL-checking the argument to "delete", it does always do that check on its own (you may rely on it, since even an overridden version of operator delete is required to do that check). Also resetting a pointer member to NULL in a destructor is not really necessary, even though static code analysis tools like "PCLint" emit warnings if you don't, for reasons that are hard to grasp (maybe to increase protection against accidental double destruction of objects holding pointers to other objects, but that has always appeared far-fetched to me).

Sven

[Dave_N]

The example explains why the "new" keyword is often overridden late in a programs development, the memory allocation with "new" is often used without checking that "new" succeeded in allocating memory. This is obviously an idyllic design achievement and most code isn't that easy to patch in the late stages of a project.

[Sven]

The example explains why the "new" keyword is often overridden late in a programs development, the memory allocation with "new" is often used without checking that "new" succeeded in allocating memory. This is obviously an idyllic design achievement and most code isn't that easy to patch in the late stages of a project.

In general I agree, where you mention the "late project stage" aspect.

But checking the result of "new" is really a separate issue, even more in a case like your example above where "new" is used within a constructor. Recently we had a brief discussion about it, also in the context of C++ exception handling (was it even a different thread?). There would be much more to say about it but it would not fit here.

Btw, "new" is a keyword but what you can override is not the keyword but the underlying "operator new" function. What the "new" keyword presents to the developer is a typed pointer while the operator always returns "void *".

Sven

[Dave_N]

Well I'll be creating 2 architectures for the Fen (and any other program feature)
a) the error checking user facade pattern (the user is always wrong!)
b) and the internal error recovery effort.

These functions needed to be optimized / tweaked at some stage anyway (PGN loading and Fen) for various reasons, and so I am repairing multiple tasks at the same time for both.

[lucasart]

Here is how I handled the fen

Not bad, but C++ is more powerful

Code: Select all

  const string PieceToChar(".PNBRQK  pnbrqk  ");
  std::istringstream fen(fenStr);
  Square sq = SQ_A8;
  char token;

  while ((fen >> token) && !isspace(token))
  {
      if (token == '/')
          sq -= Square(16); // Jump back of 2 rows

      else if (isdigit(token))
          sq += Square(token - '0'); // Skip the given number of files

      else if ((p = PieceToChar.find(token)) != string::npos)
      {
          put_piece(Piece(p), sq);
          sq++;
      }
  }

Nice ! C++ (if used well) can be pretty awesome... I have to admit
Now here's my code, so everyone can have a good laugh. I would say my code is *state of the 1970's* at least

Code: Select all

void load_fen(Board *B, const char *fen)
{
	assert(fen);
	int r, f;
	char c = *fen++;

	// load the board
	reset_board(B);
	for (r = Rank8, f = FileA; c != ' ' && r >= Rank1; c = *fen++) {
		// empty squares
		if ('1' <= c && c <= '8') {
			f += c - '0';
			assert(f <= NB_RANK_FILE);
		}
		// EOL separator
		else if (c == '/') {
			assert(f == NB_RANK_FILE);
			r--;
			f = FileA;
		}
		// piece
		else {
			int sq = square(r, f++), piece;
			for (piece = Pawn; piece <= King; piece++) {
				if (c == PieceLabel[isupper(c) ? White : Black][piece])
					break;
			}
			assert(piece_ok(piece));
			int color = isupper(c) ? White : Black;
			set_square(B, color, piece, sq, true);
			if (piece == King)
				B->king_pos[color] = sq;
		}
	}

	// turn of play
	c = *fen++;
	assert(c == 'w' || c == 'b');
	B->turn = c == 'w' ? White : Black;
	B->st->key ^= B->turn ? zobrist_turn : 0ULL;
	const int us = B->turn, them = opp_color(us);

	// Castling rights
	if (*fen++ != ' ') assert(0);
	c = *fen++;
	B->st->crights = 0;

	if (c == '-')	// no castling rights
		c = *fen++;
	else {			// set castling rights (in reading order KQkq)
		if (c == 'K') {
			B->st->crights ^= make_crights(White, OO);
			c = *fen++;
		}
		if (c == 'Q') {
			B->st->crights ^= make_crights(White, OOO);
			c = *fen++;
		}
		if (c == 'k') {
			B->st->crights ^= make_crights(Black, OO);
			c = *fen++;
		}
		if (c == 'q') {
			B->st->crights ^= make_crights(Black, OOO);
			c = *fen++;
		}
	}

	// en passant square
	assert(c == ' ');
	c = *fen++;
	if (c == '-') {
		// no en passant square
		c = *fen++;
		B->st->epsq = NoSquare;
	} else {
		int r, f;
		assert('a' <= c && c <= 'h');
		f = c - 'a';
		c = *fen++;
		assert(isdigit(c));
		r = c - '1';
		B->st->epsq = square (r, f);
	}

	B->st->pinned = hidden_checkers(B, 1, us);
	B->st->dcheckers = hidden_checkers(B, 0, us);
	B->st->attacks = calc_attacks(B, them);
	B->st->checkers = test_bit(B->st->attacks, B->king_pos[us]) ? calc_checkers(B, us) : 0ULL;

	assert(calc_key(B) == B->st->key);
	assert(calc_kpkey(B) == B->st->kpkey);
	assert(psq_ok(B));
}

However to be perfectly fair, you hid some of the code in your operator >> (that extracts std::string tokens from a std::stringstream or sth like that)

[Dave_N]

Here is how I handled the fen

Not bad, but C++ is more powerful

Code: Select all

  const string PieceToChar(".PNBRQK  pnbrqk  ");
  std::istringstream fen(fenStr);
  Square sq = SQ_A8;
  char token;

  while ((fen >> token) && !isspace(token))
  {
      if (token == '/')
          sq -= Square(16); // Jump back of 2 rows

      else if (isdigit(token))
          sq += Square(token - '0'); // Skip the given number of files

      else if ((p = PieceToChar.find(token)) != string::npos)
      {
          put_piece(Piece(p), sq);
          sq++;
      }
  }

Nice ! C++ (if used well) can be pretty awesome... I have to admit
Now here's my code, so everyone can have a good laugh. I would say my code is *state of the 1970's* at least

Code: Select all

void load_fen(Board *B, const char *fen)
{
	assert(fen);
	int r, f;
	char c = *fen++;

	// load the board
	reset_board(B);
	for (r = Rank8, f = FileA; c != ' ' && r >= Rank1; c = *fen++) {
		// empty squares
		if ('1' <= c && c <= '8') {
			f += c - '0';
			assert(f <= NB_RANK_FILE);
		}
		// EOL separator
		else if (c == '/') {
			assert(f == NB_RANK_FILE);
			r--;
			f = FileA;
		}
		// piece
		else {
			int sq = square(r, f++), piece;
			for (piece = Pawn; piece <= King; piece++) {
				if (c == PieceLabel[isupper(c) ? White : Black][piece])
					break;
			}
			assert(piece_ok(piece));
			int color = isupper(c) ? White : Black;
			set_square(B, color, piece, sq, true);
			if (piece == King)
				B->king_pos[color] = sq;
		}
	}

	// turn of play
	c = *fen++;
	assert(c == 'w' || c == 'b');
	B->turn = c == 'w' ? White : Black;
	B->st->key ^= B->turn ? zobrist_turn : 0ULL;
	const int us = B->turn, them = opp_color(us);

	// Castling rights
	if (*fen++ != ' ') assert(0);
	c = *fen++;
	B->st->crights = 0;

	if (c == '-')	// no castling rights
		c = *fen++;
	else {			// set castling rights (in reading order KQkq)
		if (c == 'K') {
			B->st->crights ^= make_crights(White, OO);
			c = *fen++;
		}
		if (c == 'Q') {
			B->st->crights ^= make_crights(White, OOO);
			c = *fen++;
		}
		if (c == 'k') {
			B->st->crights ^= make_crights(Black, OO);
			c = *fen++;
		}
		if (c == 'q') {
			B->st->crights ^= make_crights(Black, OOO);
			c = *fen++;
		}
	}

	// en passant square
	assert(c == ' ');
	c = *fen++;
	if (c == '-') {
		// no en passant square
		c = *fen++;
		B->st->epsq = NoSquare;
	} else {
		int r, f;
		assert('a' <= c && c <= 'h');
		f = c - 'a';
		c = *fen++;
		assert(isdigit(c));
		r = c - '1';
		B->st->epsq = square (r, f);
	}

	B->st->pinned = hidden_checkers(B, 1, us);
	B->st->dcheckers = hidden_checkers(B, 0, us);
	B->st->attacks = calc_attacks(B, them);
	B->st->checkers = test_bit(B->st->attacks, B->king_pos[us]) ? calc_checkers(B, us) : 0ULL;

	assert(calc_key(B) == B->st->key);
	assert(calc_kpkey(B) == B->st->kpkey);
	assert(psq_ok(B));
}

However to be perfectly fair, you hid some of the code in your operator >> (that extracts std::string tokens from a std::stringstream or sth like that)

In fact checking for a perfect FEN pasted into a program isn't as simple as that.
There is nothing pretty about my code however ... it works. The amount of rigorous checking needed isn't what I have in mind for internal board setting functions and so it is abstracted.

I simply want all the information about promotions, piece numbers first, I want to detect fantasy positions, I want to report errors to the user. So
the code simply checks every case I can think of in explicit form so that I can print the error easily ...
like this ...

Code: Select all

        // after reading the FEN ...

	if( badChar )
	{
		ERROR_MESSAGE( "Error, Bad Char found in first part of FEN string, board cleared!");
		return;
	}

	if( rank > 7 )
	{
		ERROR_MESSAGE( "Error, too many ranks (/) detected in FEN string");
		return;
	}

	for( int col = 0; col < 2; col++ )
	{
		if( pawnsCount[col] > 8 )
		{
			ERROR_MESSAGE( "Error, Too many Pawns!");
			return;
		}

		if( KingsCount[col] == 0 )
		{
			ERROR_MESSAGE( "Error, Both Sides Must Have Kings!");
			return;
		}
		if( KingsCount[col] != 1)
		{
			ERROR_MESSAGE( "Error, Too many Kings!");
			return;
		}

// ... etc
}

[Dave_N]

and if the FEN has 6 substrings separated by spaces I have code like this ...

Code: Select all

			if( vecFen[4].length() > 0 && vecFen[4].length() <= 2)
			{
				int fiftyMov = atoi(vecFen[4].c_str());
				if( fiftyMov > 50  || fiftyMov < 0 )
				{
					ERROR_MESSAGE( "Error, bad 50 move string, setting defaults!");
					fixMovNums = true;
				}
			}
			if( vecFen[5].length() > 4 || vecFen[4].length() < 1 )
			{
				ERROR_MESSAGE( "Error, move number string was corrupted, setting defaults!");
				fixMovNums = true;
			}

so each substring is checked for problems.

The internal FEN is treated like a bitboard.

[sje]

The FEN half move counter field is a count of ply, not a count of full moves. So its value can go to one hundred (or beyond if a fifty move draw goes unclaimed).