Initializing array of structures causing bus error

[Aaron M]

I'm having this annoying bus error.

I've defined two structures, and defined two types which point to these structures.

struct node {
       /* ... */
};
typedef struct node* NodeP; // NodeP points to a node.

struct graph {
        /* ... */

        NodeP* nodes; // Will be an array of node pointers.
};
typedef struct graph* GraphP; // GraphP points to a graph.

So, GraphP would point to a graph structure, which will have an array of pointers to node structures when initialized.

My problem is after I initialize <nodes> for a GraphP, I get a bus error when I try to put a NodeP into it.

This is how I create a GraphP:

GraphP newGraph(int length) {
        GraphP newGraph = (GraphP)malloc(sizeof(GraphP));

        /* Initialize other variables of newGraph. */

        newGraph->nodes = (NodeP*)calloc(length, sizeof(NodeP));

        NodeP testNode = newNode(); // A test node pointer.

        // Set all the elements of newGraph->nodes to equal testNode.
        int i;
        for(i = 0; i < length; i++)
                newGraph->nodes[i] = testNode; // Bus error here.

        return newGraph;
}

Can someone tell what's going wrong?

Thanks.

[Salem]

> typedef struct node* NodeP;
Using typedef to hide pointers is generally a bad idea IMO. For one thing, it hides the true level of indirection in several places. For another thing, it makes it much harder to add certain combinations of qualifiers (const, volatile) to the declaration.

Regarding information hiding.
node **nodes;
It is easier to see that this has two levels of indirection whereas it is a lot less obvious that this is the case with
NodeP* nodes;

> GraphP newGraph = (GraphP)malloc(sizeof(GraphP));
1. casting malloc in C does nothing for you - see the FAQ for reasons. Given some of your other code (embedded declarations), are you sure this isn't a C++ program?
2. naming the variable to be the same as the function is poor - it's called shadowing, and can lead to some rather weird scope problems (just try making your function recursive for example).

> newGraph->nodes = (NodeP*)calloc(length, sizeof(NodeP));
Why did you switch to calloc here?
It isn't adding anything special, except you get to avoid having to do a multiply yourself.

> Can someone tell what's going wrong?
Well in isolation, there is nothing wrong with the code per se, so my guess is some other malloc/free mistake elsewhere in the code is the cause, and this is just where the symptoms of the problem show up to bite you.

#include<stdio.h>
#include<stdlib.h>

typedef struct node {
  int n1;
} node;

typedef struct graph {
  node  **nodes;
} graph;

node *newNode ( void ) {
  node *result;
  result = malloc ( sizeof *result );
  return result;
}

graph *newGraph(int length) {
  graph *newGraph = malloc(sizeof *newGraph );
  node  *testNode = newNode();
  int i;

  newGraph->nodes = malloc(length * sizeof *newGraph->nodes );
  for(i = 0; i < length; i++)
    newGraph->nodes[i] = testNode;

  return newGraph;
}

int main (void)
{
  graph *new = newGraph( 10 );
  printf( "p=%p\n", (void*)new );
  return 0;
}

[Aaron M]

Thanks Salem. I made the improvements you suggested about using typdef and malloc.

I noticed that when you use sizeof you pass an actual pointer instead of a type name. Is there any particular reason for that?

[jdelator]

Not to derail the thread or anything but what does this line do?

printf( "p=%p\n", (void*)new );

[SlyMaelstrom]

Thanks Salem. I made the improvements you suggested about using typdef and malloc.

I noticed that when you use sizeof you pass an actual pointer instead of a type name. Is there any particular reason for that?

The reason you pass the variable name is because sometimes you may want to edit the program in the future to accommodate a different type. Rather than going through all of your code changing every single malloc() call to suit your new datatype you simply have to change the variable's datatype and that will affect everything. It's like an inline function that outputs a single repetitive line. You make it so it makes future edits easier.

[Salem]

Basically what Sly said.

By doing

p = malloc ( sizeof *p );  // for 1
p = malloc ( num * sizeof *p );  // for num

means you don't have to look back at the variable to see how it was declared just to allocate space, and you have less to maintain in the future.

Although it looks like you're dereferencing a pointer, remember that sizeof is a compile time operator, so the only thing being dereferenced is the type information in the compiler symbol table.

> Not to derail the thread or anything but what does this line do?
It prints a pointer - try it.