1

I want to use only studio.h library to convert from decimal number to binary number by using an array to store remainder but the result is not correct, maybe i have problem with memory allocation or return value is wrong, please help me to check it. Thank you so much!

#include <stdio.h>
int  n = 0;
int* DecimalToBinary(int number){
    int a[10];      
    while(number!=0){
        a[n++] = number%2;
        number/=2;
    }
    return a;
}

void main(){

    int *d1 = DecimalToBinary(5);
    int *d2 = DecimalToBinary(10);

    for(int i = n-1 ;i>=0;i--)
        printf(" %d",d1[i]);

    printf("\n");

    for(int i = n-1 ;i>=0;i--)
        printf(" %d",d2[i]);

}
Improve this question
5
  • "the result is not correct" - in what way? Commented Aug 6, 2013 at 7:34
  • 1
    The variable a is declared on the stack and goes out of scope when leaving that function! Commented Aug 6, 2013 at 7:35
  • A lecture why you can not do this eskimo.com/~scs/cclass/int/sx5.html Commented Aug 6, 2013 at 7:37
  • The standard function itoa does the thing you want to do; this implementation uses a static array to do that correctly (look for static in code to see how). Commented Aug 6, 2013 at 7:53
  • void main() is lazy programming. Use int main(). Commented Aug 6, 2013 at 8:05

6 Answers 6

Reset to default
8

You return a pointer to a local array. That local array is on the stack, and when the function returns the array goes out of scope and that stack memory will be reused when you call the next function. This means that the pointer will now point to some other data, and not the original array.

There are two solutions to this:

  1. Declare the array in the function calling DecimalToBinary and pass it as an argument.
  2. Create the array dynamically on the heap (e.g. with malloc) and return that pointer.

The problem with method 2 is that it might create a memory leak if you don't free the returned pointer.

As noted by Craig there is a third solution, to make the array static inside the function. However in this case it brings other and bigger problems than the two solutions I originally listed, and that's why I didn't list it.

There is also another serious problem with the code, as noted by Uchia Itachi, and that is that the array is indexed by a global variable. If the DecimalToBinary function is called with a too big number, or to many times, this global index variable will be to big for the array and will be out of bounds for the array.

Both the problem with dereferencing a pointer to an out-of-scope array and the indexing out of bounds leads to undefined behavior. Undefined behavior will, if you're lucky, just lead to the wrong result being printed. If you're unlucky it will cause the program to crash.

Improve this answer
Sign up to request clarification or add additional context in comments.

7 Comments

There is also a third solution, which is to declare a as static. This does create the problem that you can't use this function more than once, or if you do, you must first duplicate the values from the previous function call.
There is one more bug in the code. What about re-initializing the global n to 0 in the function? Everytime the array gets populated starting at differnt locations and finally goes out of bound after few function calls..
There is fourth solution to use a global array.
@UchiaItachi Using a global array leads to much the same problems as using a static local array.
Oh, and, yet another option is sticking the array in a struct and returning that. It has the benefit of not having to worry about free().
|
3

You are returning a pointer to a locally allocated array. It is allocated on the stack, and goes away when the function returns, leaving your pointer pointing to garbage.

You have a few options. You could pass an array in to fill:

void DecimalToBinary(int result[10],int number){
    while(number!=0){
        result[n++] = number%2;
        number/=2;
    }
    return result;
}

// usage example:
int b[10];
DecimalToBinary(b, 42);

Or you could allocate an array on the heap:

int* DecimalToBinary(int number){
    int *a = (int *)malloc(sizeof(int) * 10);
    while(number!=0){
        a[n++] = number%2;
        number/=2;
    }
    return a;
}

// usage example
int *b = DecimalToBinary(42);
free(b); // when finished with it

Or you could wrap the array in a struct:

typedef struct {
    int b[10];
} result;

result DecimalToBinary(int number){
    result r;
    while(number!=0){
        r.b[n++] = number%2;
        number/=2;
    }
    return r;
}

// usage example
result r = DecimalToBinary(42);

If you do the malloc() option, do not forget to free() the returned data when you're done with it, otherwise it will hang around. This is called a memory leak. In more complex programs, it can lead to serious issues.

Note: By the way, if your number is larger than 1023 (10 binary digits), you'll overrun the array. You may also wish to explicitly stop once you've stored 10 digits, or pass the size of the array in, or compute the required size first and allocate that much space. Also, you will get some odd results if your number is negative, you might want to use number&1 instead of number%2.

Note 2: As noted elsewhere, you should make n local, or at the very least reinitalize it to 0 each time the function is called, otherwise it will just accumulate and eventually you'll go past the end of the array.

Improve this answer

2 Comments

Note that in void DecimalToBinary(int result[10],int number), the "10" is going to be ignored and the array is promoted to an array. So in reality, the signature will be void DecimalToBinary(int *result, int number).
I like to include the size in the declaration for self-documentation. Just personal preference, there's no other real reason for it.
1

int[10] is not the same as int *; not only is the former created on the stack, it is a different type alltogether. You need to create an actual int * like so:

int *a = malloc (10 * sizeof (int));

Of course, don't forget to free() it after use!

Improve this answer

Comments

0

What you can also do and what is commonly done in C is creating the array where it is called and provide a pointer to that array to the function, this way when the array is on the stack of the function that calls it and not in the function self. We also have to specify the size of the array on to that function, since the function cannot know to how many elements the pointer points to

void DecimalToBinary( int number, int* output, unsigned size ) {
    /*adapt this to your liking*/
    int i;
    for ( i = 0; i < size && number != 0; i++) {
        output[i] = number%2;
        number/2;
    }
}

and in you main function you would call it like this:

int array[10];
DecimalToBinary( 5, array, sizeof(array)/sizeof(array[0]));

now array has the same result as a would have had in your example.

Improve this answer

Comments

0

The problem in your code lies here..

int * DecimalToBinary(int number){
int a[10];      
while(number!=0){
    a[n++] = number%2;
    number/=2;
}
return a;

}

The array a scope is only till this function. Once this function terminates, the memory allocated for this array will be released, either u need to use dynamic memory allocation or make array a global.

Improve this answer

Comments

-1

This is the correct program:

#include <stdio.h>
int  n = 0;
int a[10] = {0};
int* DecimalToBinary(int number){
    n = 0;     
    while(number!=0){
        a[n++] = number%2;
        number = number/2;
    }
    return a;
}

int main(){

    int *d1;
    int *d2;
    int i;
    d1 = DecimalToBinary(5);
    for(i = n-1;i>=0;i--)
        printf(" %d",d1[i]);
    printf("\n");
    d2 = DecimalToBinary(10);
    for(i = n-1;i>=0;i--)
        printf(" %d",d2[i]);
    printf("\n");
}
Improve this answer

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.