0

I am trying to pass a pointer to a function.In this function i use malloc to reserve space.The problem is that when i return in main the program doesn't respond.Here is my symplified code:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
int def(char **B){
    int i;
    B = malloc(3 * sizeof( char ));
    for(i = 0; i < 2 ; i++){
        B[i] = malloc(5 * sizeof(char));
    }
    for(i = 0; i < 2 ; i++){
        scanf("%s" , B[i]);
    }
    for(i = 0; i < 2 ; i++){
        printf("%s\n" , B[i]);
    }
    return 0;
}

int main(int argc, char *argv[]) {
    char **B;
    int i;
    def(B);
    for(i = 0; i < 2 ; i++){
        printf("%s\n" , B[i]);
    }
    return 0;
}
kuro
  • 3,214
  • 3
  • 15
  • 31
Antonis Katsikarelis
  • 21
  • 4
  • 1
    Please indent the code; – Iharob Al Asimi Apr 28 '16 at 17:04
  • You need to call free() on the pointer when you are done with it. – Samuel Apr 28 '16 at 17:14
  • Don't use magic numbers. If you have to hard-code the dimensions use `#define` so that every usage refers to a single definition. That way, it won't be so easy to allocate memory for fewer elements than there should be: and the code will be easily maintainable. And, do you realise you can only enter a 4-character string in the `scanf` statement, which is unrestricted. – Weather Vane Apr 28 '16 at 17:23

5 Answers5

2
int def(char **B)

should have been

char** def(char **B)

and its return value should have been

return B; 
 /* else the memory allocated inside the function will be freed at
  * the end and by accessing it later you have undefined behavior for the 
  * rest of the program
  */ 



B = malloc(3 * sizeof( char ));

should have been

B = malloc(3 * sizeof( char*)); // you have two levels of indirection. so char* first



for(i = 0; i < 2 ; i++) // similary with the other for loops

should have been

 for(i = 0; i < 3 ; i++) // you used 3 in the above step



def(B);

should have been

B=def(B);

It is a good practice to use free() to free the allocated memory though it will be automatically freed at the end of the program

sjsam
  • 21,411
  • 5
  • 55
  • 102
  • This doesn't fix the problem. All this does is allocate a bunch of memory which is lost when the function returns. – Tom Karzes Apr 28 '16 at 17:17
  • @TomKarzes : Didn't noticed that in the beg. but fixed it now – sjsam Apr 28 '16 at 17:26
  • `B=def(B);` - really? Passing `B`makes no sense. Further since you take the effort of writing a good answer, please also mention that malloc can fail, i.e. test the return value. – Support Ukraine Apr 28 '16 at 17:37
1

It seems you are trying to malloc something that looks like a 2D array (or rather an array of strings).

However,

def(B);

is a call by value so B doesn't change when the function returns.

If you want to change Byou need

def(&B);

and then you need to change the function signature accordingly - then you'll be a three star programmer.

If you want to do this correct then read:

Create 2D array by passing pointer to function in c

and read the answer given by @Lundin - that's the way to do it

Community
  • 1
  • 1
Support Ukraine
  • 42,271
  • 4
  • 38
  • 63
  • Then the types are wrong. This is an very incomplete answer. – Tom Karzes Apr 28 '16 at 17:19
  • @TomKarzes - exactly, that is step 2 of the problems with this code – Support Ukraine Apr 28 '16 at 17:19
  • Introducing a third level of indirection instead `return` the pointer from function to `main`. – ameyCU Apr 28 '16 at 17:20
  • @ameyCU - Agree - but that is not how the code from OP is structured. I made my answer in the intend of keeping OPs original structure. – Support Ukraine Apr 28 '16 at 17:23
  • @4386427 OP doesn't use its function even correctly . His function returns `int` but he never use that values . So , this should be taken care of (_as I don't think OP understands what he exactly wants_). And for record I did not down vote :) – ameyCU Apr 28 '16 at 17:25
  • @ameyCU - Again, agree. There are so many things to fix in this code. The purpose of my answer was/is to tell OP that he/she can't change `B` in that way and what he/she must do instead. I do not intend to rewrite the whole code to show how it should be done. I guess you don't agree with my approach. That's OK. – Support Ukraine Apr 28 '16 at 17:30
0

You seem to be trying to allocate a list of strings, but you do not provide the ability to reference this outside the def function. Part 1 of your problem is that you need to be providing a pointer to your list:

int def(char ***B) {

Another option is to return the pointer you have created:

char** def() {

Secondly in your first malloc you allocate space for 3 chars but you need char pointers. sizeof char != sizeof char*.

B = malloc(3 * sizeof( char ));

Should be:

*B = malloc(3 * sizeof char*);

Also you should use const int or #define to define your size constants so you dont accidentally get the wrong size in different places, eg.

#define SIZE_OF_LIST 3
int def(char ***B){
    int i;
    B = malloc(SIZE_OF_LIST * sizeof( char ));
    for(i = 0; i < SIZE_OF_LIST ; i++){

Finally when you pass your pointer to the function def you need to do this (top accommodate the first change):

def(&B);

or

B = def();
Fantastic Mr Fox
  • 32,495
  • 27
  • 95
  • 175
0

Actually, it is worse than that. All C function arguments are 'by value', meaning that the variables inside the function are local copies of the variables passed in at the call. So when you assign a value to an argument (which you are allowed to do, unless it is declared 'const' [I assume C++ 'const' has been adopted in C by now]), you are NOT assigning to the variable passed in at the call. So in this case the statement

B = malloc(3 * sizeof(char *));

does NOT alter the pointer B declared in main(); it just leaks memory. Assuming you really need to return an int for this function, you need to add a level of indirection:

int def(char ***B){
    *B = malloc(3 * sizeof(char *));
    for(i=0; i<2; i++){
        (*B)[i] = malloc(5 * sizeof(char));
    }
    ...
}

...

char **B;
...
def( &B );   /* Note the 'address-of' operator! */

If the return doesn't have to be int: eliminate the argument to def(), change it to return ****char**, return the result of the first malloc() directly, and change the call to B = def();

PMar
  • 1
0

You problem is that when you call def(B), you are creating a char ** that is a copy of B. If you change this new B, the changes doesn't reflect on the main function, since they were made to the copy of B.

You should use a char *** and call def(&B) (i do not consider this a good approach, tho) or you could have B initialized at the main and call def(B) to alloc and read its char * or you could return the new B.

Also, check the problems pointed in other answers.

ddz
  • 526
  • 3
  • 15