What's wrong with that code? (copying argv[] to array of ints)

Question

    #include <cstdlib>
    #include <cstdio>

    main( int argc, char **argv ) 
    {
            int *stack = new int[argc];
            int it = 0;

            while (--argc > 0 )
            {
                    *(++stack) = atoi(*++argv);     
                    printf( "%d \t %d \n", ++it, *stack );
            }

            delete stack;                   
            return 0;
    }

stack[3] should contain integer value from argv[3],but it doesn't.

What's more I get error connected with delete operator munmap_chunk(): invalid pointer

Why don't you make your code easier to read by using a simple, traditional for-loop (e.g. `for (int i = 0; i < argc; i++) { ... }`), rather than manipulating **three** variables inline on every iteration? — Oliver Charlesworth, May 31 '15 at 14:05
The pointer you call `delete` on is not the same pointer that `new` returned. Your program therefore exhibits undefined behavior. Besides, memory allocated with array form of `new` should be deallocated with array form of `delete`, as in `delete[] pointerAllocatedByNew;` — Igor Tandetnik, May 31 '15 at 14:06
You never assign `stack[0]` either. But if `argc > 3` then `stack[3]` will contain `atoi(argv[3])`. If you still have trouble then post a commandline that shows the problem — M.M, May 31 '15 at 14:33

score 2 · Accepted Answer · answered May 31 '15 at 14:13

This code isn't C; it's C++. There are two options for you:

Compile the code as C++, or ask/change your question to target the C++ audience instead. Expect them to complain about your use of printf...
Convert the code to C, which is easy enough. Change <cstdlib> to <stdlib.h>, <cstdio> to <stdio.h>, new int[argc] to malloc(argc * sizeof *stack); and delete stack; to free(stack);.

Whichever route you take, this code invokes undefined behaviour; it accesses stack out of bounds for one, and leaves the first element of stack uninitialised which I'm sure isn't to be desired. You probably meant to print the values after reading them and before incrementing stack, but since you got that wrong you're printing the next element within your array which you have of course not yet assigned...

Then to top it all off, your loop modifies the value of stack (that's what ++stack does, after all), so that after your loop when you use delete you're deleteing a reference that wasn't created using new... You need to make sure you keep the original value of stack, which gets deleted, or freed, or whatever...

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

int
main( int argc, char **argv ) {
        int *stack = malloc(argc * sizeof *stack);
        int it = 0;
        while (--argc > 0){
                stack[it] = atoi(*++argv);
                printf("%d \t %d \n", it, stack[it]);
                it++;
        }
        free(stack);
        return 0;
}

You changed the loop logic; in the original `stack[0]` was not assigned, and `stack[1]` got `argv[1]` etc. Either way I'd suggest using `it` instead of the increment and decrement operators — M.M, May 31 '15 at 14:36
@MattMcNabb Well yeh, I changed the logic so that it doesn't leave `stack[0]` uninitialised (as I mentioned in the description prior to it), and so that it doesn't attempt to access `stack` out of bounds (also mentioned earlier)... and I got accepted answer, indicating that this change of logic did indeed fix *something*. Problem? — autistic, May 31 '15 at 14:52
Thanks for response from all of you. As it's going to be an ONP calc main() function in fact should also read & recognize basic operators + - * / — makaku, May 31 '15 at 15:08

score 0 · Answer 2 · answered May 31 '15 at 14:38

Your code would be clearer (and correct) if you used array indexing instead of advancing pointers:

#include <cstdlib>
#include <cstdio>

using namespace std;

main( int argc, char **argv ) 
{
    int *stack = new int[argc];
    for( int it = 1; it < argc; ++it )
    {
        stack[it] = atoi(argv[it]);     
        printf( "%d \t %d \n", it, stack[it] );
    }

    delete[] stack;                   
    return 0;
}

No idea why you want to have unused stack[0], though.

What's wrong with that code? (copying argv[] to array of ints)

2 Answers2