why this binary conversion does not work?

Question

#include<stdio.h>
#include<conio.h>

unsigned * bin(unsigned n) {
    unsigned a[16];
    int i = 0, j = 0;
    for (i = 0; i < 16; i++) {
        a[i] = n & 0x1;
        n = n >> 1;
    }
    return a;
}

void main() {
    unsigned n = 5;
    int i = 0;
    unsigned * a = bin(n);
    for (i = 15; i >= 0; i--) {
        printf("%d\n", (*(a + i)));
    }
    getch();
}

Please help this binary conversion does not work. I'm trying to calculate x^n using binary conversion. can anybode help??

`a` has auto storage duration, and won't survive past the end of `bin`. You're returning a dangling pointer. You'll want to `malloc` (or `new` up) an array to return, and free (or `delete[]`) it in the caller. Or, in C++, use a `std::vector` instead, which will take care of the mundane memory stuff for you. — cHao, May 08 '13 at 15:33
First thing to do (before posting here) is probably to read the compiler warnings. Your compiler will typically catch the problem with returning local variables. — Mattias Nilsson, May 08 '13 at 15:36
Should I add that `` is not only non-portable, but deprecated on the few systems which support it, and that `main` is required to have `int` as its return type. — James Kanze, May 08 '13 at 15:37

score 6 · Answer 1 · answered May 08 '13 at 15:33

6

You are returning a pointer to a local variable. This variable is stored on the stack, and will not be valid after the function returns.

Dereferencing this pointer will lead to undefined behavior.

The solution is to either make the variable static, or pass in the array as an argument to the function, or (as noted in a comment by James Kanze) use a type that copies the contents.

answered May 08 '13 at 15:33

Some programmer dude

400,186
35
402
621

1

Use `std::vector`. Both as return value and the local variable. – James Kanze May 08 '13 at 15:35
Otherwise: passing in the array is the best of the C solutions I've seen to date. – James Kanze May 08 '13 at 15:38
2

While using a static variable is technically correct, it's a bad idea for other reasons. Please don't do that. – Mark Ransom May 08 '13 at 15:39
@MarkRansom As Posix has learned:-). Learn from them, and pass in the array. – James Kanze May 08 '13 at 15:42

MOHAMED · Answer 2 · 2013-05-08T15:38:11.703

2

you can not return a local array defined in the function in this way.

The content of the array will be erased when the function finish the execution.

instead of using

 unsigned a[16];

you can use the following:

unsigned *a =malloc(16 * (sizeof *a));

And do not forget in your main to free the memory allocated for a when the a array become useless in your program. you can free the array with:

free(a);

edited May 08 '13 at 15:38

answered May 08 '13 at 15:32

MOHAMED

41,599
58
163
268

You can if you declare the array `std::vector` (the normal way to declare an array in C++). – James Kanze May 08 '13 at 15:36
didnot u mean unsigned *a=(unsigned*)malloc(16 * (sizeof *a)); – hayees May 08 '13 at 15:39
@user2362994 [Do not cast the malloc result](http://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc) – MOHAMED May 08 '13 at 15:41
@MOHAMED Don't use `malloc` to begin with, use `std::vector` (and forget about C). But if you have to use C, the cast is perfectly valid, and recommended by some of the better C programmers. – James Kanze May 08 '13 at 15:43
@user2362994 You can use it directly without casting as mentioned in the answer – MOHAMED May 08 '13 at 15:44
@JamesKanze Concerning the `std::vector`, the OP source code looks like pure C code. so I give my solution in C and not C++. – MOHAMED May 08 '13 at 15:46
it is giving me this error Error BIN.C 7: Cannot convert 'void *' to 'unsigned int *' in function bin(unsigned int) – hayees May 08 '13 at 15:47
@user2362994 what happen when you use the cast `unsigned a=(unsigned)malloc(16 * (sizeof *a));` Does the compilation work without problems ? – MOHAMED May 08 '13 at 15:48
@user2362994 so keep the casting – MOHAMED May 08 '13 at 15:51
@user2362994 if you are compiling your source as C++ so you have to keep the casting . and if you are compiling your source as C then you can remove the casting – MOHAMED May 08 '13 at 15:59
how about my other question – hayees May 08 '13 at 16:08
the question is not clear. Try to open new question for that and try to clear more the question and provide what you have tried – MOHAMED May 08 '13 at 16:14

Mats Petersson · Answer 3 · 2013-05-08T15:49:35.637

Actually, this is a typical case where using new (or malloc) is a pretty bad choice. However, as others have said, returning a pointer to a local array is bad.

Instead, pass in an array:

void bin(unsigned n, unsigned a[]) {
    int i = 0;
    for (i = 0; i < 16; i++) {
        a[i] = n & 0x1;
        n = n >> 1;
    }
}

and in main:

unsigned a[16];
bin(n, a);

Now you have no need to allocate or return an array from bin.

why this binary conversion does not work?

3 Answers3