Thread: Problem with vector of structures and dynamic allocation

  1. #1
    Registered User
    Join Date
    Sep 2011
    Posts
    78

    Problem with vector of structures and dynamic allocation

    Code:
    struct comp *comps = 0;
    struct comp *ccompx(const char name[])
    {
    struct comp *p;
    struct comp *pa;
    glob = 0;
    pa = (struct comp*) malloc(sizeof(struct comp));
    if(!pa)
    {
        puts("Ricerca interrotta.");
        return 0;
    }
    int s;
    for(p = comps, s = 1; p; p = p->n, s++)
    {
        int i = 0;
    for(; name[i] != p->name[i]; i++);
    if(p && name[i] == p->name[i])
    {
        int a = 1;
        for(; name[i + a] == p->name[i + a]; a++);
        if(a == strlen(name))
        {
            pa[s - 1] = p; // error
            glob++;
            realloc(pa, sizeof(struct comp) * s + 1);
            if(!pa)
            {
                puts("Ricerca non completata.")
                goto ret;
            }
        }
        return 0;
    }
    }
    ret:
    return pa;
    }
    Why pa[s - 1] = p, doesn't work?

  2. #2
    Registered User
    Join Date
    Nov 2010
    Location
    Long Beach, CA
    Posts
    5,909
    There are a number of problems with this code, but I'll cover your question first:

    p and pa is of type pointer to struct comp. When you index into pa (which you treat like an array of struct comps), you end up with a single struct comp. You can not assign a pointer-to-struct-comp to a struct comp, they are incompatible. If your compiler warnings are turned up, you should get an error similar to:

    vec.c:28: error: incompatible types when assigning to type ‘struct comp’ from type ‘struct comp *’

    As for the other issues:
    • glob and comps appear to be a global variables. Global variables are evil, avoid them at all costs. Read this: Global Variables Are Bad.
    • Don't cast malloc. Read the FAQ: Cprogramming.com FAQ > Casting malloc.
    • for(; name[i] != ...) Style issue, but it would be clearer if you put i = 0 there instead of in the declaration.
    • if (p && name[i]...) You don't need to check if p is valid, the for loop guarantees it is.
    • Your for(; name[i + a]...) and the subsequent if(a == ...) could be replaces with something like if(strcmp(name, p->name) == 0) // if name and p->name are the same).
    • Your use of realloc isn't safe. pa will not be set to NULL if realloc fails, you have to check the return value for that. You to store the return value in a temp variable and check if that is null before reassigning pa to the new value, otherwise, you can lose any reference to the allocated memory, meaning data loss and memory leak.

  3. #3
    Algorithm Dissector iMalc's Avatar
    Join Date
    Dec 2005
    Location
    New Zealand
    Posts
    6,318
    Quote Originally Posted by drew99 View Post
    Why pa[s - 1] = p, doesn't work?
    "doesn't work" - ah the two words that we should never ever hear from someone asking for help. Instead, EXPLAIN what it actually does.

    In this case, it wont compile because p is a pointer. Try dereferencing it.
    My homepage
    Advice: Take only as directed - If symptoms persist, please see your debugger

    Linus Torvalds: "But it clearly is the only right way. The fact that everybody else does it some other way only means that they are wrong"

  4. #4
    Registered User
    Join Date
    Sep 2011
    Posts
    78

    RE

    I made ​​the following changes:

    Code:
    struct comp *p = (struct comp*)malloc(sizeof(struct comp));
        if(!p)
        {
            //
        }
        struct comp **pv = 0;
        struct comp ***point;
        int s;
        for(p = comps, s = 1; p; p = p->n, s++)
        {
            unsigned int i = 0;
            for(; name[i] != p->name[i]; i++);
            if(p && name[i] == p->name[i])
            {
                unsigned int a = 1;
                for(; name[i + a] == p->name[i + a]; a++);
                if(a == strlen(name))
                {
                    pv = (struct comp **) realloc(pv, sizeof(struct   comp*) * (s + 1));
                    if(!pv)
                    {
                        goto ret;
                    }
                    point = &pv;
                    pv[s] = p;
                }
                continue;
            }
        }
        pv[0]->code = s; //element 0 works as a counter of the total elements
    ret:
        return point;
    }
    If I wanted to give back to the function this array of structures, so it can be used in the main function, what changes should I make at the code?
    Last edited by drew99; 09-01-2011 at 04:57 PM.

  5. #5
    and the Hat of Guessing tabstop's Avatar
    Join Date
    Nov 2007
    Posts
    14,336
    Is there a reason you're making an array out of your linked list (or, alternatively, making a linked list of your array)?

    If not, you should just make a normal linked list (which means no pv) and point is useless even if you want an array.

    Normally you head of a linked list is passed around from place to place as "the" linked list.

  6. #6
    Registered User
    Join Date
    Sep 2011
    Posts
    78
    If you read the code, you can see that compx is a research function of structures belonging to a linked list. The function inserts the results (structures) in an array and passes them to the main function that processes the vector and prints the results.

  7. #7
    and the Hat of Guessing tabstop's Avatar
    Join Date
    Nov 2007
    Posts
    14,336
    Right, yes, sorry; I read one of those "name" as "next" and I was way off in never-land and never came back.

    In that case then just returning the malloc'ed/realloc'ed pointer will provide access to the array.

  8. #8
    Registered User
    Join Date
    Sep 2011
    Posts
    78
    I've tried this:

    Code:
    struct point **vector = compx(//name of component);
    if(vector[0])
    {
    int i = 0;
    for(; pv[0]->code > i++;)
                    {
                        printf("\nCode of the component:           %d\n", pv[e]->code);   // comp
                        printf("Name of the component:             %s\n", pv[e]->name);  // structure's
                        printf("Dessciption of the component:      %s\n", pv[e]->des);    // elements
                        printf("Disponibility of the component:   %d\n\n", pv[e]->disp); 
                    }
    }
    The code doesn't work: the results are nosense int or strings.
    Last edited by drew99; 09-02-2011 at 03:22 AM.

  9. #9
    Banned
    Join Date
    Aug 2010
    Location
    Ontario Canada
    Posts
    9,547
    And where is e set to a useable value?

    Also...
    Code:
    int i = 0;
    for(; pv[0]->code > i++;)
    What's with the screwy for() loops?

  10. #10
    Registered User
    Join Date
    Sep 2011
    Posts
    78

    RE

    I modified the function:

    Code:
    struct comp ***compx(const char name[]);
    so in the main():

    Code:
    struct point ***pv = compx(//name of component);
    if((*pv)[0])
    {
    int e = 0;
    for(;(*pv)[[0]->code > e++;)
                    {
                        printf("\nCode of the component:           %d\n", (*pv)[i]->code);
                        printf("Name of the component:             %s\n", (*pv)[i]->name);
                        printf("Dessciption of the component:      %s\n", (*pv)[i]->des);
                        printf("Disponibility of the component:   %d\n\n", (*pv)[i]->disp); 
                    }
    }
    The program returns the error when I try to access only to (*pv)[0]->code.

  11. #11
    Registered User
    Join Date
    Nov 2010
    Location
    Long Beach, CA
    Posts
    5,909
    Code:
    struct comp ***compx(const char name[]);
    Uh-oh, despite what you might think, being a three-star programmer is generally not a good thing. Why are you creating a pointer to a pointer to a pointer to a struct point? If you don't do sufficient mallocs for each level of indirection, you will be messing with memory you don't on and you'll get errors for things like (*pv)[0]. Also, as far as I can tell, compx returns a pointer to a pointer to a pointer to a struct comp, but you assign that to a pointer to a pointer to a pointer to a struct point. That's no good, they aren't the same thing.

    Quote Originally Posted by drew99
    ...compx is a research function of structures belonging to a linked list...
    I assume by "reasearch", you mean a search function? So you're searching a linked list? Why does it return a triple pointer to a struct comp, instead of just a pointer to a struct comp?

    I'm really confused by what it is you actually want to do. You need to explain more clearly exactly what you want. You also need to provide more complete code. What does a struct comp look like? What about a struct point? What does the new version of compx() look like? etc, etc, etc.

  12. #12
    Registered User
    Join Date
    Nov 2010
    Location
    Long Beach, CA
    Posts
    5,909
    Quote Originally Posted by drew99 View Post
    Code:
    struct point ***pv = compx(//name of component);
    if((*pv)[0])
    {
    int e = 0;
    for(;(*pv)[[0]->code > e++;)
                    {
                        printf("\nCode of the component:           %d\n", (*pv)[i]->code);
                        printf("Name of the component:             %s\n", (*pv)[i]->name);
                        printf("Dessciption of the component:      %s\n", (*pv)[i]->des);
                        printf("Disponibility of the component:   %d\n\n", (*pv)[i]->disp); 
                    }
    }
    The program returns the error when I try to access only to (*pv)[0]->code.
    What do you think happens to that red line if pv is NULL? Yep, *pv fails. What about if *pv is NULL? Yep, (*pv)[0] fails. You should check pv and *pv before dereferencing them or indexing into them.

  13. #13
    Registered User
    Join Date
    Sep 2011
    Posts
    78
    This the code:

    Code:
    struct comp ***ccompx(const char name[])
    {
        struct comp **pv = (struct comp**) malloc(sizeof(struct comp*));
        if(!pv)
        {
            puts("\nRicerca interrotta");
        }
        pv[0] = comps;
        struct comp ***point;
        struct comp *p = (struct comp*)malloc(sizeof(struct comp));
        if(!p)
        {
            puts("\nRicerca interrotta");
        }
        int s = 1;
        for(p = comps; p; p = p->n)
        {
            if(strstr(p->name, name))
            {
                pv = (struct comp **) realloc(pv, sizeof(struct comp*) * (s + 1));
                if(!pv)
                {
                    puts("\nRicerca non completata");
                    goto ret;
                }
                point = &pv;
                (*point)[s] = p;
                s++;
            }
        }
        if(--s == 0)
        {
            free(point);
            point = 0;
            return point;
        }
        (*point)[0]->code = s;
    ret:
        return point;
    }
    
    int main()
    If (*pv)[0] is null the code passes to else.

  14. #14
    Registered User
    Join Date
    Nov 2010
    Location
    Long Beach, CA
    Posts
    5,909
    Quote Originally Posted by drew99 View Post
    This the code:
    That is only part of the code. Show me more! Show me the definition of struct comp and struct point!

    You run around, assigning from struct comp *** to struct point ***, but those aren't the same type. Your compiler should be yelling at you for this. If it isn't, turn up the warnings, and fix them all.

    If (*pv)[0] is null the code passes to else.
    Yes, we can see that from your example. But before you can do (*pv)[0], the computer has to be able to do *pv. And before it can do *pv, it must have a valid address in pv. If pv is invalid (NULL), then *pv is an invalid memory access, and may crash your program. If pv is valid, but *pv is invalid (NULL), then (*pv)[0] will crash your program. It's the dereferencing of pv and the indexing into (*pv) that will crash it, not if (*pv)[0] is NULL. Here is what you need to do:
    Code:
    if (pv && *pv && (*pv)[0]) {  // make sure pv and *pv are valid before trying (*pv)[0]
    }

  15. #15
    Registered User
    Join Date
    Sep 2011
    Posts
    78
    Code:
    struct comp
    {
        int code;
        char name[25];
        char des[100];
        int disp;
        struct comp *n;
    };
    
    struct comp *comps = 0;
    Struct point doesn't exist(struct comp ***point). Point works as a pv's pointer. Because of the realloc instruction, the function needs a pointer to pv.

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Basic dynamic memory allocation problem
    By Crosshash in forum C Programming
    Replies: 11
    Last Post: 03-22-2011, 01:14 PM
  2. Dynamic memory allocation problem
    By linuxlover in forum C Programming
    Replies: 17
    Last Post: 11-25-2010, 01:11 PM
  3. Problem with custom dynamic memory allocation routines
    By BLauritson in forum C++ Programming
    Replies: 12
    Last Post: 03-11-2010, 07:26 AM
  4. Link List,Structures,Dynamic Memory Allocation,Pointers
    By Tanuj_Tanmay in forum C Programming
    Replies: 2
    Last Post: 04-24-2009, 08:55 PM
  5. dynamic memory allocation problem
    By firyace in forum C Programming
    Replies: 4
    Last Post: 05-23-2007, 09:57 PM