views:

374

answers:

4

Please don't close this saying a duplicate question. I made an important change so not to confuse people, I resubmitted this much clearer codes.

Please help me to solve this memory allocation problem. I'm working on a HashTable and this is what I've (partial codes only)

main.c

HashTablePtr hash;
hash = createHashTable(10);
insert(hash, "hello");
insert(hash, "world");

HashTable.c

    HashTablePtr createHashTable(unsigned int capacity){
    HashTablePtr hash;
    hash = (HashTablePtr) malloc(sizeof(HashTablePtr));
    hash->size = 0;
    hash->capacity = capacity;          
    hash->list = (ListPtr *)calloc(capacity, sizeof(List)); /*NO MEMORY ALLOCATION HERE*/
    return hash;

List.h

typedef struct list List;
typedef struct list * ListPtr;

struct list {
    int size;
    NodePtr head;
    NodePtr tail;
};
...
...

HashTable.h

    typedef struct hashtable * HashTablePtr;
    typedef struct hashtable HashTable;
    struct hashtable {
        unsigned int capacity;
        unsigned int size;
        ListPtr *list;
        unsigned int (*makeHash)(unsigned int, void *);
    };
...
...

When I run my debugger (Netbeans C/C++ debugger, I see no memory being allocated to hash->list. In above example, my attempt is to make it an array of 10 lists.

Please help me to solve this.

I'm not that expert in C, if that helps.

+4  A: 

This line

hash = (HashTablePtr) malloc(sizeof(HashTablePtr));

should read

hash = (HashTablePtr) malloc(sizeof(HashTable));

I also suggest you follow the advice that was given to you in your previous post and NOT hide pointers by typedef'ing. Just use List *

[Whenever you see an alloc statement, the type you assigning to is a pointer, and the type you are allocating should be of the type (and not the pointer to the type)]

Mitch Wheat
I understand that Mitch but the problem is I've got the List.h and List.c files from my professor and can't messup with that. I tried converting HashTablePtr to HashTable but without any luck.
Then I would (after getting your assignments marks!) suggest to the 'professor' (anonymously?) that he not confuse his students and foloow industry best practice.
Mitch Wheat
we already discussed this issue in the class and the professor says "that's how I learned C and that's the good practice. For me the codes are cleaner because I don't have to deal with a bunch of asterisks."
Or simply ignore the typedefs and use List * anyway. They're equivalent, so it'll still work with the prof's code.
bdonlan
@newbie: Your professor has probably never written or had to maintain production code.
Mitch Wheat
@bdonlan: that's true, but I had the feeling (correctly it seems) that his professor would mark down if he used List *
Mitch Wheat
+1  A: 

In this line:

hash->list = (ListPtr *)calloc(capacity, sizeof(List));

The calloc function will allocate some memory for List, then return a pointer to the allocated memory, which would be List* (or, equivalent to ListPtr due to the typedef.)

In the code, the pointer is then casted to (ListPtr*), which would actually be List** which is not the type List* which is expected. Therefore, change the ListPtr* to ListPtr and see if that would fix that line.

Edit

As pointed out by leiz and Klathzazt in the comments, the Hashtable.list type is ListPtr* or List**:

struct hashtable {
    ...
    ListPtr *list;

This would be the reason why trying to assign a ListPtr type returned and casted from calloc would cause a compiler error. Rather than storing a pointer to a pointer of a List in the Hashtable, it should just hold a pointer to the list:

struct hashtable {
    ...
    ListPtr list;

This should eliminate the compiler error, as the type of Hashtable.list and typecast both will be ListPtr.

coobird
I get this warning if I tried to replace ListPtr * with ListPtr:HashTable.c:15: warning: assignment from incompatible pointer type
your hash table "ListPtr *list;" should also be "ListPtr list"
leiz
in your struct as well- you have to replace ListPtr * with ListPtr. As previously stated, it would be a lot less confusing if you did not have the typedef statements.
Klathzazt
ok that seems to make it a bit better but with that how can I be able to make an array of 10 lists?
+4  A: 

One nice idiom that you can use as well is eg:

hash = (HashTablePtr) malloc(sizeof(*hash));

By not hard-coding any types in the allocation, the possibility of mixing things up is greatly reduced.

Note that 'hash' is not actually dereferenced here - sizeof() is always evaluated at compile time, so the compiler just figures out what type *hash /would/ have, and gets the size of that.

bdonlan
+1 for saving me having to post that note on use of sizeof. Also, IMO the return value of malloc() should never by cast in C, it's just pointless and silly.
unwind
True, but for all we know his prof is having him code C in a C++ compiler, so I just left it in there.
bdonlan
A: 

Personally I am not a huge fan of using typedefs, especially when you are a beginner. I think that may be partially what is confusing you. You are better avoiding things like:

typedef struct hashtable * HashTablePtr;

Using to many typedefs will make your code harder to read since you will need to constantly look up what they are referring too.

The main problem is that you were allocating the memory for the size of a hashtable/list pointer and not for the size of their respected structures. I think the code below shows this well. You will also want to check if you allocations worked. If malloc, calloc, realloc. etc. fail they return NULL. If this happens and you do not check for this case you will get a segfault error and your program will crash.

Also follow the c99 standard, and put all of your variable declarations at the start of the function.

c99 std

malloc manpage

struct hashtable *
createHashTable(unsigned int capacity){
    struct hashtable *hash;
    struct list *mylist;

    /* You want to allocate size of the hash structure not the size of a pointer. */
    hash = malloc(sizeof(struct hashtable)); 
    // always make sure if the allocation worked.
    if(hash == NULL){
        fprintf(stderr, "Could not allocate hashtable\n");
        return NULL;
    }

    hash->size = 0;
    hash->capacity = capacity;

    /* Unless you need the memory to be zero'd I would just use malloc here
     * mylist = calloc(capacity, sizeof(struct list)); */
    mylist = malloc(capacity * sizeof(struct list));
    if(mylist == NULL){
        fprintf(stderr, "Could not allocate list\n");
        free(hash); /* free our memory and handle the error*/
        return NULL;
    }

    mylist->head = NULL;
    mylist->size = 0;
    mylist->tail = NULL;    
    hash->list = mylist;

    return hash;
}

Also remember to free you list before you free your hashtable:

free(myhash->list);
free(myhash);
Sweeney
You should indicate which section of the C99 standard you're referring to. It's a big document.
bk1e
I wasn't referring to a specific section, just that its a good document to have and read.
Sweeney