1

I have a simple exercise to do on C. But when I initialize my variable graph with malloc, it do not perform the action correctly. Here is my code:

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

typedef int Weight;

typedef struct aux {
    int vDest;
    Weight weight;
    struct aux * next;
} TypeEdge;

typedef TypeEdge* TypePointer;

typedef struct {
    TypePointer * listAdj;
    int numVertices;
    int numEdges;
} TypeGraph;

typedef int* TipoInt;

bool initializeGraph(TypeGraph *graph, int nv) {
    if (nv < 0) {
        return false;
    }

    graph->numVertices = nv;
    graph->numEdges = 0;
    int i;
    for (i = 0; i < nv; i++) {
        graph->listAdj = (TypePointer*) malloc(sizeof(TypePointer));
    }
    for (i = 0; i < nv; i++) {
        graph->listAdj[i] = NULL;
    }
    return true;
}

void insertEdge(int v1, int v2, Weight weight, TypeGraph *graph) {
    if (v1 < 0 || v1 > graph->numVertices || v2 < 0 || v2 > graph->numVertices) {
        return;
    }
    TypePointer actual = graph->listAdj[v1];
    while (actual->next) {
        actual = actual->next;
    }
    TypePointer pNew = (TypePointer) malloc(sizeof(TypeEdge));
    pNew->vDest = v2;
    pNew->weight = weight;
    pNew->next = NULL;
    actual->next = pNew;
}

int main() {
    TypeGraph graph;
    bool result = initializeGraph(&graph, 100);
    if (result) {
        insertEdge(2, 3, 1, &graph);
    }
    return 0;
}

The problem is that, instead of initializing the graph with size of one hundred TypePointer, it initializes just size two and do not perform any action. When I try to debug it on Clion it shows this error message: read memory from 0x4000000000000000 failed (0 of 4 bytes read). If I just run the code it returns code eleven.

Please, can someone help me?

Caio Ambrosio
  • 61
  • 1
  • 7

2 Answers2

0

You really have a big mess on your hands. Contributed to by your typedef'ing of pointers, but primarily because you fail to allocate nv pointers, but instead allocate the same listAdj pointer and then immediately overwrite the pointer with NULL creating 100 memory leaks.

Think about it:

    for (i = 0; i < nv; i++) {
        graph->listAdj = (TypePointer*) malloc(sizeof(TypePointer));
    }

(There is no need to cast the return of malloc, it is unnecessary. See: Do I cast the result of malloc? and... ALWAYS validate EVERY allocation)

graph->listAdj is a single-pointer that holds the address to the newly allocated block of memory each iteration that is then overwritten by each subsequent allocation.

Further, you then attempt to dereference the overwritten pointer and set pointers not in existence to NULL, e.g.:

    for (i = 0; i < nv; i++) {
        graph->listAdj[i] = NULL;
    }

You only attempted to allocate a single graph->listAdj (albeit 100 times), not nv pointers as you intended. You must allocate storage for nv pointers at once.

Now let's start at the beginning and clean things up by remove ALL typedefs, and simply using int for bool, e.g.

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

typedef struct aux {
    int vDest;
    int weight;
    struct aux *next;
} TypeEdge;

typedef struct {
    TypeEdge **listAdj;
    int numVertices;
    int numEdges;
} TypeGraph;

Now it is plainly clear to anyone looking at your code that listAdj is a pointer-to-pointer to TypeEdge. There is no guessing or having to hunt around later wondering what TypePointer 300 lines later, TypeEdge is TypeEdge, nothing more.

When you initializeGraph, you need to allocate all nv pointers is a single call to malloc, not in a loop. Then you can loop over the pointers setting them all NULL, e.g.

int initializeGraph (TypeGraph *graph, int nv)
{
    if (nv < 0)
        return 0;

    graph->numVertices = nv;
    graph->numEdges = 0;
    int i;

    if ((graph->listAdj = malloc(sizeof *graph->listAdj * nv)) == NULL) {
        perror ("malloc-graph->listAdj");
        return 0;
    }

    for (i = 0; i < nv; i++)
        (graph->listAdj)[i] = NULL;

    return 1;
}

Next when you insertEdge(), you must handle the case where you are inserting the first edge for that vertex, or whether you need to iterate to the end of the list and insert there. You also need to adjust how you iterate to the end to ensure you do not attempt to access actual->next if actual is NULL. Putting that together, you can do:

TypeEdge *insertEdge (int v1, int v2, int weight, TypeGraph *graph) 
{
    if (v1 < 0 || v1 > graph->numVertices || 
        v2 < 0 || v2 > graph->numVertices) {
        return NULL;
    }

    TypeEdge *actual = graph->listAdj[v1];
    while (actual && actual->next)
        actual = actual->next;

    TypeEdge *pNew = malloc(sizeof *pNew);
    if (!pNew) {
        perror ("malloc-pNew");
        return NULL;
    }

    pNew->vDest = v2;
    pNew->weight = weight;
    pNew->next = NULL;

    if (!actual)
        graph->listAdj[v1] = pNew;
    else
        actual->next = pNew;

    return (pNew);
}

(note: how the return type of the function has been changed to TypeEdge * from void so you have a meaningful return that can indicate Success/Failure of your attempt to insert the edge. Never allocate within a void function without a way of indicating to the calling function whether the allocation (and remainder of critical steps) have succeeded or failed)

While your main() attempts to insertEdge(), it doesn't give you any indication of what happened. Unless you have some way of validating you actually inserted the edge, it kind of leaves you wondering. Just write a short set of print functions to handle outputting the list of edges for each vertex that has them, e.g.

void prnedge (const TypeEdge *e)
{
    do
        printf (" %3d %3d\n", e->vDest, e->weight);
    while ((e = e->next));
}

void print_edge (const TypeEdge *e, int edge)
{
    printf ("\nedge %d\n", edge);
    prnedge (e);
}

void print_graph (const TypeGraph *g)
{
    for (int i = 0; i < g->numVertices; i++)
        if (g->listAdj[i])
            print_edge (g->listAdj[i], i);
}

If you have allocated memory, you need to be able to free that memory as well. A similar short set of functions can handle freeing each list, e.g.

void freelist (TypeEdge *l)
{
    while (l) {
        TypeEdge *victim = l;
        l = l->next;
        free (victim);
    }
}

void free_graphlists (TypeGraph *g)
{
    for (int i = 0; i < g->numVertices; i++)
        if (g->listAdj[i])
            freelist (g->listAdj[i]);

    free (g->listAdj);
}

Which you can call in main() as:

int main (void) {

    TypeGraph graph;
    int result = initializeGraph (&graph, 100);

    if (result) {
        insertEdge (2, 3, 1, &graph);
        insertEdge (2, 4, 1, &graph);
    }

    print_graph (&graph);
    free_graphlists (&graph);

    return 0;
}

Putting it altogether, you could do:

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

typedef struct aux {
    int vDest;
    int weight;
    struct aux *next;
} TypeEdge;

typedef struct {
    TypeEdge **listAdj;
    int numVertices;
    int numEdges;
} TypeGraph;

int initializeGraph (TypeGraph *graph, int nv)
{
    if (nv < 0)
        return 0;

    graph->numVertices = nv;
    graph->numEdges = 0;
    int i;

    if ((graph->listAdj = malloc(sizeof *graph->listAdj * nv)) == NULL) {
        perror ("malloc-graph->listAdj");
        return 0;
    }

    for (i = 0; i < nv; i++)
        (graph->listAdj)[i] = NULL;

    return 1;
}

TypeEdge *insertEdge (int v1, int v2, int weight, TypeGraph *graph) 
{
    if (v1 < 0 || v1 > graph->numVertices || 
        v2 < 0 || v2 > graph->numVertices) {
        return NULL;
    }

    TypeEdge *actual = graph->listAdj[v1];
    while (actual && actual->next)
        actual = actual->next;

    TypeEdge *pNew = malloc(sizeof *pNew);
    if (!pNew) {
        perror ("malloc-pNew");
        return NULL;
    }

    pNew->vDest = v2;
    pNew->weight = weight;
    pNew->next = NULL;

    if (!actual)
        graph->listAdj[v1] = pNew;
    else
        actual->next = pNew;

    return (pNew);
}

void prnedge (const TypeEdge *e)
{
    do
        printf (" %3d %3d\n", e->vDest, e->weight);
    while ((e = e->next));
}

void print_edge (const TypeEdge *e, int edge)
{
    printf ("\nedge %d\n", edge);
    prnedge (e);
}

void print_graph (const TypeGraph *g)
{
    for (int i = 0; i < g->numVertices; i++)
        if (g->listAdj[i])
            print_edge (g->listAdj[i], i);
}

void freelist (TypeEdge *l)
{
    while (l) {
        TypeEdge *victim = l;
        l = l->next;
        free (victim);
    }
}

void free_graphlists (TypeGraph *g)
{
    for (int i = 0; i < g->numVertices; i++)
        if (g->listAdj[i])
            freelist (g->listAdj[i]);

    free (g->listAdj);
}

int main (void) {

    TypeGraph graph;
    int result = initializeGraph (&graph, 100);

    if (result) {
        insertEdge (2, 3, 1, &graph);
        insertEdge (2, 4, 1, &graph);
    }

    print_graph (&graph);
    free_graphlists (&graph);

    return 0;
}

Example Use/Output

$ ./bin/edgetype

edge 2
   3   1
   4   1

Memory Use/Error Check

In any code you write that dynamically allocates memory, you have 2 responsibilities regarding any block of memory allocated: (1) always preserve a pointer to the starting address for the block of memory so, (2) it can be freed when it is no longer needed.

It is imperative that you use a memory error checking program to insure you do not attempt to access memory or write beyond/outside the bounds of your allocated block, attempt to read or base a conditional jump on an uninitialized value, and finally, to confirm that you free all the memory you have allocated.

For Linux valgrind is the normal choice. There are similar memory checkers for every platform. They are all simple to use, just run your program through it.

$ valgrind ./bin/edgetype
==21679== Memcheck, a memory error detector
==21679== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==21679== Using Valgrind-3.12.0 and LibVEX; rerun with -h for copyright info
==21679== Command: ./bin/edgetype
==21679==

edge 2
   3   1
   4   1
==21679==
==21679== HEAP SUMMARY:
==21679==     in use at exit: 0 bytes in 0 blocks
==21679==   total heap usage: 3 allocs, 3 frees, 832 bytes allocated
==21679==
==21679== All heap blocks were freed -- no leaks are possible
==21679==
==21679== For counts of detected and suppressed errors, rerun with: -v
==21679== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

Always confirm that you have freed all memory you have allocated and that there are no memory errors.

Look things over and let me know if you have further questions.

David C. Rankin
  • 81,885
  • 6
  • 58
  • 85
0

There are somethings wrong in your initializeGraph and insertEdge. This change should be work for you

bool initializeGraph(TypeGraph *graph, int nv)
{
    if (nv < 0) {
        return false;
    }

    graph->numVertices = nv;
    graph->numEdges = 0;

    /* call `calloc` to avoid using for-loop to zero memory */
    graph->listAdj = calloc(nv, sizeof(TypePointer));

    return true;
}

void insertEdge(int v1, int v2, Weight weight, TypeGraph *graph)
{
    if (v1 < 0 || v1 > graph->numVertices || v2 < 0 || v2 > graph->numVertices) {
        return;
    }

    /* just insert the node at the head of the adjList */
    TypePointer pNew = malloc(sizeof(TypeEdge));

    pNew->vDest = v2;
    pNew->weight = weight;
    pNew->next = graph->listAdj[v1];
    graph->listAdj[v1] = pNew;
    graph->numEdges++;
}

As for typedef, please read Why should we typedef a struct so often in C? and The Linux Kernel CodingStyle document to make your own decision whether you should use it or not. Personally, I myself avoid using typedef unless necessary.

Paul
  • 1,630
  • 1
  • 16
  • 23