Re: [gnugo-devel] my beloved bamboo joins again

[Paul Pogonyshev]

Martin wrote:
> After some experimenting, I have come up with the following, which seems
> a little over-specific, as it only solves 13x13:65 (well, and my initial
> example, of course). But on the bright side, it has almost no
> performance impact: the totals of regress.pike are within +- 0.05%, some
> of the test-suites also run faster.

Sounds good.

If you want to continue contributing to GNU Go, we'll ask you to assign
copyright on your changes to Free Software Foundation.  Are you willing to
do this?

Below are some style related comments on your patch.  Please don't consider
them as an offence, everybody have seen them regarding his first patches,
including me ;)

We'll probably accept the patch anyway, but this means extra work of cleaning
later (typically for Gunnar or maybe for me).  If you fix the style problems
and send us a rediffed patch, it will be better.

> +  for (l1 = 0; l1 < num_libs; ++l1)
> +    for (l2 = 0; l2 < num_libs; ++l2) {

It is a sort of an unsaid convention in GNU Go to use post-increment ("l1++")
when both give the same result.  Not so important, though.

> +      if (l1==l2) continue;

Please add spaces around most operators, in particular, around all comparison
operators.  Even more important, please never place "then" branch of `if' on
the same line, even if it is very simple.  In other words, the statement above
should have looked this way:

        if (l1 == l2)
          continue;

> +      switch (libs[l1]-libs[l2]) {
> +     case 1:
> +     case -1:
> +       if (board[SOUTH(libs[l1])] == EMPTY
> +           && board[SOUTH(libs[l2])] == color
> +           && !is_self_atari(SOUTH(libs[l1]), color))
> +         ADD_CANDIDATE_MOVE(SOUTH(libs[l1]), 0, *moves, "bamboo_rescue");
> +       if (board[NORTH(libs[l1])] == EMPTY
> +           && board[NORTH(libs[l2])] == color
> +           && !is_self_atari(NORTH(libs[l1]), color))
> +         ADD_CANDIDATE_MOVE(NORTH(libs[l1]), 0, *moves, "bamboo_rescue");
> +       break;
> ...

Although this is not forced so strictly, we prefer `case' to be at the same
indentation levels as `switch'.  You can see it a couple of times in
`reading.c'.  If you use Emacs, just put your point on a line with `case',
hit C-c C-o and inter "0" for indentation offset, then reindent your
function.

Another point: i don't quite like these case labels, they are go to deep
into board internals, i'd say.  You could have just written

        if (libs[l1] == WEST(libs[l2]) || libs[l1] == EAST(libs[l2])) {
          ...
        }
        else if (libs[l1] == SOUTH(libs[l2] || libs[l1] == NORTH(libs[l2])) {
          ...
        }

This seems cleaner to me.

There is also a common trick with using delta[] array (delta[(k + 1) % 4] is
always perpendicular to delta[k]), but it doesn't seem to fit here easily.
So, the way you wrote it, only with `switch' replaced with `if' looks best
(to me).

Happy new year
  Paul