Re: [gnugo-devel] board.c:3319 - color == WHITE || color == BLACK near F16

[Gunnar Farneback]

Stuart wrote:
> I've attached a patch. It's been a long while since I coded C, so 
> feedback on the coding style is welcome.

The coding style in the sources is mostly uniform. When in doubt,
look for something similar to imitate. I'll comment on the deviations
I find here.

> I'm not sure whether I've made the patch correctly, but it seems to
> contain all the necessary information.

The patch is backwards. When new stuff is added you should get lots of
lines starting with plus signs. I would also recommend the unified
diff format (diff -u) which is easier to parse when reviewing the
patch.


- int
- sgfnode_expandmultiple(SGFNode *node){

Put the starting brace of the function on a new line.

-   int i,j;

Always one space after the comma.

-   SGFProperty * oldprops = NULL; // the old property list

No space between pointer asterisk and variable.

Do not use C++-style comments since those are not included in the C89
standard.

-   if (node == NULL) return 0;

Put the body of the if statement on a new line.

-     if ((strlen(oldprops->value) == 5 && oldprops->value[2] == ':') &&
-       (oldprops->name == SGFAB ||
-        oldprops->name == SGFAW ||
-        oldprops->name == SGFAE ||
-        oldprops->name == SGFTW ||
-        oldprops->name == SGFTB)) {

Place the && and || at the beginning of the next line instead of at
the end.

-       char c = oldprops->value[3];
-       char d  = oldprops->value[4];

I assume the extra space here was an accident.

-       if (a > c) {
-       c = oldprops->value[0];
-       a = oldprops->value[3];
-       }
-       if (b > d) {
-       d  = oldprops->value[1];
-       b = oldprops->value[4];
-       }

Tabs line up somewhat confusingly in patches (due to the extra first
column) but I believe there's only a one-space indentation here.

-       for (i = a; i<=c;i++) {

We write these as
        for (i = a; i <= c; i++) {

-         } else {

New line for else.

-     } else {
-       if (newprops == NULL){
-       newprops = oldprops;
-       tail = oldprops;
-      } else {
-        tail->next = oldprops;
-        tail = oldprops;
-       }

There's definitely something strange going on with the indentations
here.

/Gunnar