C++ pointer function causes empty parameter

Question

I'm making a virtual machine in C++. I have loaded in the contents of a file as a string. I pass this string to a function of type int*, but the problem is the string variable containing the contents of the file seems to be empty because when I try to use cout << file << endl; I get nothing.

Here is the file in question:

#include <iostream>
#include <string>
#include <fstream>
#include <sstream>

using namespace std;

class reedoovm {

    private:
        string filedata;
        string instruction;
        string file;
        int instr;
        int instructionCount;
        int instructionPointer;

    public:
        int load_program(string filename) {
            ifstream rdfile(filename);
            while(rdfile >> instruction) {      /* Get each instruction */
                    filedata += instruction;        /* Append the instruction to filedata */
                    filedata += ",";                /* Append a comma to separate each instruction */
                    instructionCount++;
                }
            rdfile.close();                         /* Close the file */
            return instructionCount;                        /* Return the filedata */
        }

        int *instrToArr(string file) {
            //file = "02,0022,00E1,0022,00,04,73";
            cout << file << endl;
            stringstream hextoint;
            unsigned int value;
            string s = file;                      /* store fconv in a variable "s" */
            string delimiter = ",";                /* The delimiter */
            size_t pos = 0;
            string token;
            int i = 0;
            int inst;
            static int* instarray;
            instarray = (int*) calloc(instructionCount,sizeof(int));
            while ((pos = s.find(delimiter)) != string::npos) {     /* Convert hex instructions to decimal */
                token = s.substr(0, pos);
                stringstream hextoint(token);
                hextoint >> hex >> value;
                if (i < instructionCount) {
                    instarray[i] = value;
                    cout << instarray[i] << endl;
                    i++;
                }
                s.erase(0, pos + delimiter.length());
            }
            return instarray;
        }

        int getNextIntruction(string s) {
            int *instruction = instrToArr(s);
            cout << *instruction << endl;
            return 0;
        }

        void run_program(string s) {
            int loop = 1;
            while (loop) {
                instr = getNextIntruction(s);
                loop = 0;
            }
        }

        void execute_program(string s) {
            file = load_program(s);
            int * arr = instrToArr(file);
            //cout << arr << endl;
            //run_program(s);
        }       

};

int main(int argc, char* argv[]) {
    reedoovm rd;
    rd.execute_program(argv[1]);
    return 0;
}

The function causing the problem is int *instrToArr(string file) {. I don't know why all of a sudden the file variable is empty.

file = load_program(s); are you sure that this does even compile? — Till
– Till, Commented Apr 26, 2014 at 16:48
@till: Unfortunately it's perfectly legal to assign an integer to a string in C++ (no warnings, no errors). It's called strong typing ;-) — 6502
– 6502, Commented Apr 26, 2014 at 16:53
@KonradRudolph, I agree. I'd prefer leaving it at file.assign(1, someChar) to match the constructor. I suppose file = {someChar}; would be explicit as well. — Qaz
– Qaz, Commented Apr 26, 2014 at 16:57

6502 · Accepted Answer · 2014-04-26 17:35:27Z

1

Your code has many issues, but the one that is bugging you is probably

file = loadProgram(s);

because loadProgram has been defined as returning an integer (the number of instructions) and not a string, but you're assigning it to a string.

For what I'd call a design bug of C++ assigning an integer to a string is a perfectly legal instruction and means that the string will have one character with the value of the integer.

Officially the reason for accepting assignment from an integers is that it was thought that it could be useful to write

str += chr; // Adds the char at the end

where str is a string and chr a char. By extension if += was legal then it was thought that also assignment should be legal too (a logical jump I don't agree with in this specific case).

chars however in C++ are numbers and integers (or even doubles) can be converted implicitly to a char without any warning or any error. So it's for example also legal:

std::string s;
s = 3.141592654;

Other issues I can see in your code are:

1. `instructionCount` is not initialized

In C++ you must always initialize native type members (e.g. integers, doubles) in class instances in the constructor. The default constructor won't do it for you. The result is that when allocating the class instance those members will have random values and you don't want that. Official explanation for this rule is that initializing members that won't be access may penalize performance, if the programmer wants to pay for initialization then it has to write the initialization.

2. `instrToArr` returns a pointer to a local `static` variable

That variable that is however allocated each time the function is called thus leaking memory at each call if the caller doesn't take care of deallocation.

Note that in C++ writing:

static int * instarray = (int *)calloc(...);

is not the same as writing:

static int * instarray;
instarray = (int *)calloc(...);

because in the first case the allocation is done only once (the first time the code reaches that instruction) while in the second case the allocation is done every time.

3. You are using `calloc`

Your code is allocation a variable-sized array using calloc and this, while not a bad idea in absolute, requires very careful handling to avoid leaks or other errors (for example memory allocated with calloc must be freed with free and not with delete[] but the compiler cannot help the programmer remembering what was allocated with one or with the other method (new[]).

MUCH better unless there are very specific reasons to play with naked pointers and implicit sizes is to use std::vector for variable-sized arrays.

4. You seem to want hex -> int conversion

... but your code does nothing to do it. Unfortunately input parsing is a sad story in C++ and I, as one, prefer to use old c <stdio.h> functions for input and especially for output (where formatting in C++ is just too painful).

5. your `getNextInstruction` always returns 0

Nothing remains of the processing of instrToArr and also the array returned is just dropped on the floor after sending the address on output. This means just leaking memory at every iteration.

6. your `run_program` just loops once

... thus at least the naming is confusing (there are no real loops).

7. your program doesn't do any kind of checking in `main`

If someone calls the program passing no arguments (a quite common case) then something bad is going to happen.

edited Apr 26, 2014 at 17:35

answered Apr 26, 2014 at 17:00

6502

115k17 gold badges177 silver badges277 bronze badges

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

4 Comments

user3566150 Over a year ago

Can you tell me the other problems so I can fix them please? I don't mind constructive criticism if it makes my program better. :D

6502 Over a year ago

@user3566150: there are MANY of them... if you promise you won't take it personally I can write a complete analysis of this code from my personal point of view.

user3566150 Over a year ago

I promise I won't take it personally! I'll take all the advice I can get!

user3566150 Over a year ago

Thanks for your advice! I'll adjust my code accordingly. :D

rockoder · Accepted Answer · 2014-04-26 16:55:16Z

1

I think in load_program() instead of:

return instructionCount;

you meant:

return filedata;

And change the return type of load_program() to string

answered Apr 26, 2014 at 16:55

rockoder

74711 silver badges23 bronze badges

Comments

ujohnny · Accepted Answer · 2014-04-26 16:57:55Z

1

I suppose you have a typo

int * arr = instrToArr(file)

instead of

int * arr = instrToArr(filedata)

answered Apr 26, 2014 at 16:57

ujohnny

1111 silver badge2 bronze badges

Collectives™ on Stack Overflow

C++ pointer function causes empty parameter

3 Answers 3

1. `instructionCount` is not initialized

2. `instrToArr` returns a pointer to a local `static` variable

3. You are using `calloc`

4. You seem to want hex -> int conversion

5. your `getNextInstruction` always returns 0

6. your `run_program` just loops once

7. your program doesn't do any kind of checking in `main`

4 Comments

Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

1. instructionCount is not initialized

2. instrToArr returns a pointer to a local static variable

3. You are using calloc

4. You seem to want hex -> int conversion

5. your getNextInstruction always returns 0

6. your run_program just loops once

7. your program doesn't do any kind of checking in main

4 Comments

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related

1. `instructionCount` is not initialized

2. `instrToArr` returns a pointer to a local `static` variable

3. You are using `calloc`

5. your `getNextInstruction` always returns 0

6. your `run_program` just loops once

7. your program doesn't do any kind of checking in `main`