Messing up realloc

[Clockowl]

Hey guys,

This program gives me a nice segfault (on line 27 and if it's removed on the realloc() below that) and I can't seem to figure out why. Any help is greatly appreciated.

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

/*
  Parses parse_this for tokens using strtok() with seperators
  (but doesn't alter parse_this due to internal copy) and puts
  tokens into array_of_strings.

  array_of_string (out): should be a NULL pointer in the beginning.
  nr_of_strings (out)  : contains the number of strings
  parse_this (in)      : string to be parsed
  seperators (in)      : seperators to be passed to strtok()
*/

int cexplode(char ***array_of_strings, unsigned int *nr_of_strings, const char * const parse_this, const char * const seperators) {
    unsigned int loc_nr_of_strings = 0;
    unsigned int n = 0;
    unsigned int sepcnt = 0;

    for (sepcnt = 0; sepcnt < strlen(seperators); sepcnt++) {
        for (n = 0; n < strlen(parse_this); n++) {
            if (parse_this[n] == seperators[sepcnt]) loc_nr_of_strings++;
        }
        loc_nr_of_strings++;
    }
    printf("DEBUG: %p | %d\n", (*array_of_strings), loc_nr_of_strings);

    (*array_of_strings) = realloc((*array_of_strings), loc_nr_of_strings * sizeof(char*));
    memset((*array_of_strings), 0, loc_nr_of_strings * sizeof(char*));

    char *parse_this_copy = calloc(sizeof(char), strlen(parse_this) + 1);
    strcpy(parse_this_copy, parse_this);

    char *pch = strtok(parse_this_copy, seperators);

    n = 0; //string count
    while (pch != NULL) {
        //allocate, copy, print, next string
        (*array_of_strings)[n] = realloc((*array_of_strings)[n], strlen(pch) + 1);
        memset((*array_of_strings)[n], 0, strlen(pch) + 1);
        strcpy((*array_of_strings)[n], pch);
        n++;
        pch = strtok (NULL, seperators);
    }

    //loc_nr is for alloc. loc_nr can be > true nr. of strings, n is always true nr of strings.
    loc_nr_of_strings = n;
    (*nr_of_strings) = loc_nr_of_strings;

    return 0;
}

int main(int argc, char *argv[]){
    char ***restest = NULL;
    char *test = "chickens\\hide\\in\\great\\numbers\\and\\eat\\brocolli\\iiiiiiiiii\\iiii\\iiiii";
    unsigned int counter = 0;

    cexplode(restest, &counter, test, "\\");

    unsigned int i = 0;
    for(i = 0; i < counter; i++){
        puts(restest[i]);
    }

    return 0;
}

Thanks In advance,
Nick

[Salem]

Does it also give you warning when you compile?
> puts(restest);
This line should have complained.

> char ***restest = NULL;
Just because your function accepts a char*** means that you should declare your input parameter in the same way.

In this instance, you should have done

char **restest = NULL;
cexplode(&restest, &counter, test, "\\");

You want an expression who's type is char***, not necessarily a variable with that type.

> for (sepcnt = 0; sepcnt < strlen(seperators); sepcnt++)
The string is a constant, so calling strlen() every time is only going to waste time, because it always returns the same result.

> (*array_of_strings)[n] = realloc((*array_of_strings)[n], strlen(pch) + 1);

Each is already NULL, so just call calloc() like you did for this:
> char *parse_this_copy = calloc(sizeof(char), strlen(parse_this) + 1);
Also for parse_this_copy, you need to call free()

[Clockowl]

Ah yes, weird.. Weird.

I disagree with this:

"The string is a constant, so calling strlen() every time is only going to waste time, because it always returns the same result."

I'm expecting my compiler will do that for me, it seems like a very simple optimization to make, right?

The rest is right on the money, thanks a bunch.

[Aia]

[...]I disagree with this:

"The string is a constant, so calling strlen() every time is only going to waste time, because it always returns the same result."

I'm expecting my compiler will do that for me, it seems like a very simple optimization to make, right?

The rest is right on the money, thanks a bunch.

calling strlen() in the for loop will invoke an instance of strlen() every time it loops. As it has been pointed out, an unnecessary waste of CPU cycles.
Call strlen() once outside the for loop and store the result in a size_t variable.

[ajay.krish123]

There are three main errors:
1.

(*array_of_strings) = realloc((*array_of_strings), loc_nr_of_strings * sizeof(char*));

this cannot convert void into char therefore the modified one is

(**array_of_strings) = realloc((*array_of_strings), loc_nr_of_strings * sizeof(char*));

2.similar to first error

char *parse_this_copy = calloc(sizeof(char), strlen(parse_this) + 1);

should be

char *parse_this_copy =(char*)calloc(sizeof(char),strlen(parse_this) + 1);

3.The code written

puts(restest);

should be

puts(*restest[i]);

[Clockowl]

That void* to char* isn't even giving me a warning with -Wall in GCC so I suspect that it's not necessary at all. This isn't C++.

Don't forget that void means (afaik) "lack of type", so you can just convert it to anything if you want to. No such thing as "can't convert from void to..." in C I think.

And you're first fix doesn't make sense. The fixes by Salem are correct.

Oh and final thanks to Salem and Aia because the optimization sped it up about.. 25 times. (with in GCC -O3, and that expensive optimizations mode). Yeah. That's sorta embarrassing, but I'm glad I followed your advice.

[ajay.krish123]

>> No such thing as "can't convert from void to..." in C I think.
This problem was shown in my Borland compiler so it might depend on the type of compiler one uses.

[Salem]

> This problem was shown in my Borland compiler so it might depend on the type of compiler one uses.
Specifically, whether you're compiling the code using a C compiler (void* conversion is implicit), or a C++ compiler (void* needs a cast).