I just updated to the latest master and made the change, and the assert still fires. Also, regarding see_sign: Why must it be better? The condition in see_sign cannot possibly be true in this context except in the CASTLING case. The rest of the time, it will be (slightly) slower than see.

EDIT: In fact, the precise effect of this pull request is to revert this commit: 187a9fe

I am impressed, wow.

I have two remarks:

1. The command to check whether asserts fire is the following?
   make build debug=yes ARCH=x86-64-modern

I let it run until depth 26 from start position without any problems. Am I doing something wrong?

2. The patches are functionally identical and there were no crashes in 135k games, so what could ever go wrong?

Actually, I was looking at the first patch (removal of the type == NORMAL condition) without the see_sign change. With see_sign, the assert won't fire for the reason given in the description of the linked commit.

But for me neither fires.

That's odd. To answer your earlier question, I use Visual C++ 2015 and the assert fires in debug mode. One can logically see that it should fire without see_sign though. Consider white's kingside castling in standard chess (not Chess960), the move would be encoded as e1->h1 (king captures rook), so the reverse move passed to see would be encoded as h1->e1, both of which will be empty following a castling. So when see calls color_of(piece_on(from)) this is color_of(piece_on(H1)) == color_of(NO_PIECE). The assert should fire.

see_sign() was replaced by see() in this patch: 187a9fe

Knowing the background now, I am quite neutral about the patch. Test has showed that there is nothing to be scared of. If mantainers do not want to commit, could you @mcostalba integrate the comment to line 1008? Otherwise we will make the same discussion in July 2017...

Ah, and now I see: the assert fires for me too for the version with see. I must have done something wrong last time.

@Stefano80 I agree we can add one comment to current code to better explain why it is done in that way (check only normal moves and use see instead of see_sign). Eventually you can use this pull request for this: add the comment and revert the code to the original one.

Ok, if none objects. I will do as soon as Lucas tests are finished. Maybe in the meanwhile mantainers can express their opinion.

What you can do is this:
type_of(pos.piece_on(to_sq(move))) != KING

No need to call the function.... As any king movement including castling will not be <0.
This should be a small speed up.

Using see_sign() will only trigger if it's a king movement. But adding the code above... using see_sign() will be redundant as it will always call see().

@VoyagerOne That won't work, as to_sq of a castling move is not the king destination but the rook origin, so it will be empty in standard chess (and only nonempty in certain Chess960 positions).

@DU-jdto
Are you certain about that?
Here is the proposed code to make sure we are on the same page:
if ( r
&& type_of(pos.piece_on(to_sq(move))) != PAWN
&& type_of(pos.piece_on(to_sq(move))) != KING //proposed code
&& pos.see(make_move(to_sq(move), from_sq(move))) < VALUE_ZERO)
r = std::max(DEPTH_ZERO, r - ONE_PLY);

AFTER making the castling move. The piece on to_sq should definitely be the king.

Looking under the hood:
position.cpp; line 910
to = relative_square(us, kingSide ? SQ_G1 : SQ_C1);

You would be correct if it was BEFORE we make the move.

@VoyagerOne
to_sq isn't where the king ultimately ends up getting moved to, it is nothing more or less than the value of the low six bits of the move's 16-bit representation, which at generation is set to the location of the rook, not the destination square of the king. See movegen.cpp:

Square kfrom = pos.square<KING>(us);
Square rfrom = pos.castling_rook_square(Cr);
...
Move m = make<CASTLING>(kfrom, rfrom);

Where make is implemented in type.cpp as:

template<MoveType T>
inline Move make(Square from, Square to, PieceType pt = KNIGHT) {
  return Move(to | (from << 6) | T | ((pt - KNIGHT) << 12));
}

Indeed, this fact is why the assert fires at all, because to_sq(move) refers to an empty square in the CASTLING case. Just to be absolutely sure, I've tried your proposed change, and the assert still fires. You should find the same if you try it.

@DU-jdto
I understand now...I wrongly assumed that move would get updated when do_move is called.

@Stefano80, @mcostalba:

I agree with Marco that a clarifying comment would be the best.

Suggested actions:

• Close this pull
• Open another pull request with only comment updated

Ok, I do as suggested.