0

I'm trying to add items to a dynamic array using a function. What I'm doing is calculating the size of the array and increasing it by one. My problem is in calculating the size of the array; sizeof(*tab) / sizeof((*tab)[0]) always gets zero.

struct table
{
    char key[20];
    int val;
};

struct table *symTab, *insTab, *datTab;

void addFieldToTable(struct table **tab, char key[MAX_KEY_LENGTH], int val) {
    struct table temp;
    strcpy(temp.key, key);
    temp.val = val;

    size_t space;
    if (*tab == 0) {
        space = 1;
        *tab = malloc(space);
        *tab[0] = temp;
        return;
    }
    //#region for test only
    size_t sizeOfTab = sizeof(*tab);//4
    size_t sizeOfEntity1 = sizeof((*tab)[0]);//24
    size_t sizeOfEntity2 = sizeof(**tab);//24
    size_t sizeOfEntity3 = sizeof(struct table);//24
    //#endregion
    space = sizeof(*tab) / sizeof((*tab)[0]);//always gets zero
    space++;
    *tab = realloc(*tab, space);
    *tab[space - 1] = temp;

}

int main()
{
    addFieldToTable(&insTab, "mov", 0);
    addFieldToTable(&insTab, "cmp", 1);
}

In my code, the first item is added successfully, but when trying to add the second one the calculated size is zero, so the array doesn't resize, and so the first item *tab[0] is replaced by the new item.

How can I solve this problem of calculating array size to making the function able to add items dynamically?.

Abozanona
  • 2,261
  • 1
  • 24
  • 60
  • you are doing it completely wrong. you are allocating 1byte space for table struct pointer? impllement a linked list and then create instance of table struct in addFieldToTab function and add that instance to linked list. – not_python Aug 10 '17 at 03:37
  • and *tab is a pointer to struct table. how can you use index operator for that? – not_python Aug 10 '17 at 03:39
  • as junaid said, you are allocating 1 byte to the `tab` whereas the size of `table` is greater than it. And dereferencing the pointer to this struct is undefined behaviour I believe. Your `tab` must be an array of structs in order to use index operator on it. Are you sure of what you are doing here? – WhiteSword Aug 10 '17 at 04:04
  • Fyi you might find the [operator precedence](http://en.cppreference.com/w/c/language/operator_precedence) taking place in an expression like `*tab[space - 1] = ...` to be differing than what you're assuming, and believe me, it makes a difference. – WhozCraig Aug 10 '17 at 04:20
  • Possible duplicate of [Sizeof arrays and pointers](https://stackoverflow.com/questions/13672162/sizeof-arrays-and-pointers) – n. m. could be an AI Aug 10 '17 at 06:19

3 Answers3

1

You may be making things much harder on yourself than they need to be. For starters, you can always reduce the effort required to keep track of your currently allocated number of table structs, by adding a simply wrapper around your data that includes a counter. You can also use typedef here to your advantage.

For example, instead of handling table directly, why not move the elements of table to a simple struct of its own, say tbl_data and simply include a pointer in table to the data and a counter for the number of tbl_data elements allocated? It could work something like this:

typedef struct {
    char key[MAX_KEY_LENGTH];
    int val;
} tbl_data;

typedef struct {    /* simple wrapper preserving allocation size in ntbl */
    tbl_data *tbl;
    size_t ntbl;
} table;

Next, avoid using global variables. You are already passing **tab as a parameter, there is no need to declare insTab as a global. Declare it in main() as pass it as a parameter as it appears you intended to do. (note: using the wrapper, you can use a simple static instance of table and pass it as *tab)

Think about the structure scheme above. You are only allocating for each tbl_data added, and after allocating and validating the new memory, you simply assign your temp to tab->tbl[tab->ntbl] and increment the number of tbl_data (tab->ntbl) by 1. Much simpler. And since you are increasing the allocation by 1 with each addFieldToTable, you can just use realloc for the first and all subsequent allocations simplifying addFieldToTable.

(note: repeatedly calling malloc (or realloc or calloc) for every element added is not a very efficient allocation scheme, but it is fine for now. You can allocate blocks of 32, 512 and realloc when tbl->ntbl reaches the limit, or just double tbl->ntbl each time and realloc when your reach the new limit to reduce the realloc calls)

If you declare table insTab = { NULL, .ntbl = 0 }; in main(), your addFieldToTable with improved validation can be reduced to:

void addFieldToTable (table *tab, char *key, int val) 
{
    if (!key) {     /* validate key is not NULL */
        fprintf (stderr, "error: key parameter is NULL\n");
        return;
    }

    tbl_data temp;
    size_t keylen = strlen (key);   /* get the length of key */

    if (keylen > MAX_KEY_LENGTH) {  /* truncate if > MAX_KEY_LENGTH */
        key[keylen - 1] = 0;
        fprintf (stderr, "warning: key exceeds %d, truncated.\n", 
                MAX_KEY_LENGTH);
    }

    strcpy (temp.key, key);
    temp.val = val;

    /* just realloc tbl_data, since you are increasing by 1 each time */
    void *tmptbl = realloc (tab->tbl, (tab->ntbl + 1) * sizeof *(tab->tbl));

    if (!tmptbl) {  /* validate realloc succeeded or handle error */
        fprintf (stderr, "error: memory exhausted at '%zu' elements.\n",
                tab->ntbl);
        return;
    }
    tab->tbl = tmptbl;              /* assign new block to *tab */

    tab->tbl[tab->ntbl] = temp;     /* assign temp    */
    tab->ntbl++;                    /* increment ntbl */
}

(note: you should really change your function type to tbl_data* and return a pointer to tab->tbl so you can (1) return an indication of success/failure, e.g. the pointer or NULL; and (2) make a pointer to the new tbl_data immediately available. You could also just declare it as type int and return 1 for success and 0 on failure and keep the benefits of (1) above)

Putting it altogether, you could do something similar to the following:

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

#define MAX_KEY_LENGTH 20

typedef struct {
    char key[MAX_KEY_LENGTH];
    int val;
} tbl_data;

typedef struct {    /* simple wrapper preserving allocation size in ntbl */
    tbl_data *tbl;
    size_t ntbl;
} table;

void addFieldToTable (table *tab, char *key, int val) 
{
    if (!key) {     /* validate key is not NULL */
        fprintf (stderr, "error: key parameter is NULL\n");
        return;
    }

    tbl_data temp;
    size_t keylen = strlen (key);   /* get the length of key */

    if (keylen > MAX_KEY_LENGTH) {  /* truncate if > MAX_KEY_LENGTH */
        key[keylen - 1] = 0;
        fprintf (stderr, "warning: key exceeds %d, truncated.\n", 
                MAX_KEY_LENGTH);
    }

    strcpy (temp.key, key);
    temp.val = val;

    /* just realloc tbl_data, since you are increasing by 1 each time */
    void *tmptbl = realloc (tab->tbl, (tab->ntbl + 1) * sizeof *(tab->tbl));

    if (!tmptbl) {  /* validate realloc succeeded or handle error */
        fprintf (stderr, "error: memory exhausted at '%zu' elements.\n",
                tab->ntbl);
        return;
    }
    tab->tbl = tmptbl;              /* assign new block to *tab */

    tab->tbl[tab->ntbl] = temp;     /* assign temp    */
    tab->ntbl++;                    /* increment ntbl */
}

int main (void)
{
    table insTab = { NULL, .ntbl = 0 };

    addFieldToTable (&insTab, "mov", 0);
    addFieldToTable (&insTab, "cmp", 1);
    addFieldToTable (&insTab, "next command", 1234);

    if (insTab.ntbl == 0) {
        fprintf (stderr, "error: insTab is empty.\n");
        exit (EXIT_FAILURE);
    }

    for (size_t i = 0; i < insTab.ntbl; i++)
        printf ("%-*s : %d\n", MAX_KEY_LENGTH, 
                insTab.tbl[i].key, insTab.tbl[i].val);

    free (insTab.tbl);  /* don't forget to free allocated memory */

    return 0;
}

Example Use/Output

$ ./bin/struct_table
mov                  : 0
cmp                  : 1
next command         : 1234

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 write beyond/outside the bounds of your allocated block of memory, 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/struct_table
==4003== Memcheck, a memory error detector
==4003== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==4003== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info
==4003== Command: ./bin/struct_table
==4003==
mov                  : 0
cmp                  : 1
next command         : 1234
==4003==
==4003== HEAP SUMMARY:
==4003==     in use at exit: 0 bytes in 0 blocks
==4003==   total heap usage: 3 allocs, 3 frees, 144 bytes allocated
==4003==
==4003== All heap blocks were freed -- no leaks are possible
==4003==
==4003== For counts of detected and suppressed errors, rerun with: -v
==4003== 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 any questions.

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

This line space = sizeof(*tab) / sizeof((*tab)[0]);//always gets zero is indeed terrible!

tab is declared to be a struct table **, so *tab is a pointer and sizeof(*tab) is only the size of a pointer... Assuming that struct table contains more than one single element, it is likely that sizeof(struct table) > sizeof(struct table *), so the integer division is normally 0.

So you must keep the allocated size yourself (you have no direct way to get it from C). But anyway, a realloc can be an expensive operation, so you should allocate in chunks and keep both the allocated size and the used size.

Serge Ballesta
  • 143,923
  • 11
  • 122
  • 252
0

You are allocating 1B for struct which has 24B + padding.

What you have to do is allocate memory for sizeof your struct.

*tab = malloc(sizeof(struct table));

sizeof's

sizeof(*tab); // Is just size of pointer

sizeof(**tab); === sizeof(struct table);  // It's size of Structure

If you want to find out how many element's does array have, pass another parameter with that info.

void addFieldToTable(struct table **tab, char key[MAX_KEY_LENGTH], int val, int noElements)

int noElements = 0;
addFieldToTable(&insTab, "mov", 0, noElements++);
addFieldToTable(&insTab, "cmp", 1, noElements++);
addFieldToTable(&insTab, "third", 1, noElements++);

Reallocation is expensive operation, you should for example allocate space for 100 structures, and then, if you need more space, reallocate for 200 structures.

If you cant figure it out, there is correction of your code

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

#define MAX_KEY_LENGTH 20
struct table
{
    char key[MAX_KEY_LENGTH];
    int val;
};

struct table *symTab, *insTab, *datTab;

void addFieldToTable(struct table **tab, char key[MAX_KEY_LENGTH], int val, int noElements) {
    struct table temp;
    strcpy_s(temp.key, key);
    temp.val = val;

    if (*tab == NULL)
    {
        *tab = malloc(sizeof(struct table));
        (*tab)[0] = temp;
        return;
    }

    *tab = realloc(*tab, ((noElements + 1) * sizeof(struct table)));

    (*tab)[noElements] = temp;
}

int main()
{
    int noElements = 0;
    addFieldToTable(&insTab, "mov", 0, noElements++);
    addFieldToTable(&insTab, "cmp", 1, noElements++);
    addFieldToTable(&insTab, "third", 1, noElements++);

    for (int i = 0; i < noElements; i++)
    {
        printf("\n%d: %s, %d", i+1, insTab[i].key, insTab[i].val);
    }

    return 0;
}

OUTPUT

1: mov, 0
2: cmp, 1
3: third, 1
kocica
  • 6,412
  • 2
  • 14
  • 35