C++ vector problem

I'm getting some weird behavior with a vector in C++ I was hoping someone could help me out. I have a vector like so:

vector<Instruction*> allInstrs; 

the struct for Instruction is as follows:

struct Instruction : simple_instr
{
    InstrType type;
    Instruction(const simple_instr& simple) : simple_instr(simple) 
    {
        type = Simple;
        loopHeader = false;
        loopTail = false;
    }
    int Id;
    bool loopHeader;
    bool loopTail;
};

the problem I'm having is this:

I need to iterate through each instruction and pull out specific fields and use those to do some analysis on the instructions in the vector. To do that, I was basically doing

VariableList Variables;

void GenerateVariableList()
    {
        for (int i = 0; i < allInstrs.size(); i++)
        {           
             Variables.Add(allInstrs[i]);
        }
        Variables.RemoveDuplicates();
    }

Variable List is defined as

struct VariableList
{
    void Add(simple_instr* instr)
    {
        PrintOpcode(instr);
        switch(instr->opcode)
            {
                case STR_OP:
                case MCPY_OP:
                    Add(instr->u.base.src1);
                    Add(instr->u.base.src2);
                    break;              
                case LDC_OP: 
                    Add(instr->u.ldc.dst);
                    break;              
                case BTRUE_OP:
                case BFALSE_OP: 
                    Add(instr->u.bj.src);
                    break;              

                case CALL_OP:
                    cout << "CALL OP" <<endl;
                    break;

                case MBR_OP:                    
                    Add(instr->u.mbr.src);
                    break;          

                case RET_OP:
                    if (instr->u.base.src1 != NO_REGISTER)
                        Add(instr->u.base.src1);
                    break;              

                case CVT_OP:
                case CPY_OP:
                case NEG_OP:
                case NOT_OP: 
                case LOAD_OP: 
                    Add(instr->u.base.dst);
                    Add(instr->u.base.src1);
                    break;

                case LABEL_OP:
                case JMP_OP:
                    break;                      

                default:
                    Add(instr->u.base.dst);
                    Add(instr->u.base.src1);
                    Add(instr->u.base.src2);
                    break;

            }
    }

    void Add(Variable var)
    {
        variableList.push_back(var);
    }

    void RemoveDuplicates()
    {
        if (variableList.size() > 0)
        {
            variableList.erase(unique(variableList.begin(), variableList.end()), variableList.end());
            currentID = variableList.size();
        }
    }

    VariableList()
    {
        currentID = 0;
    }

    VariableList(VariableList& varList, bool setLiveness = false, bool LiveVal = false)
    {
        currentID = 0;
        for (int i = 0; i < varList.size(); i++)
        {
            Variable var(varList[i]);
            if (setLiveness)
            {
                var.isLive = LiveVal;
            }
            variableList.push_back(var);
        }
    }

    Variable& operator[] (int i)
    {
        return variableList[i];
    }

    int size()
    {
        return variableList.size();
    }

    vector<Variable>::iterator begin()
    {
        return variableList.begin();
    }

    vector<Variable>::iterator end()
    {
        return variableList.end();
    }

    protected:
        int currentID;
        vector<Variable> variableList;

        void Add(simple_reg* reg, bool checkForDuplicates = false)
        {   cout << "Register Check" <<endl;
            if (reg == null)
            {
                cout << "null detected" << endl;
                return;
            }
            if (reg->kind == PSEUDO_REG)
            {   

                if (!checkForDuplicates || (checkForDuplicates && find(variableList.begin(), variableList.end(), reg->num) != variableList.end()))
                {
                    cout << "Adding... Reg  " << reg->num << endl;
                    Variable var(reg->num, currentID);

                    variableList.push开发者_Python百科_back(var);
                    currentID++;
                }
            }
        }

};

When I do this though, every instruction goes to the default case statement, even though I knwo for a fact some instructions shouldn't. If I change GenerateVariableList to

void GenerateVariableList()
    {
        for (int i = 0; i < allInstrs.size(); i++)
        {   
            PrintOpcode(allInstrs[i]);
            Variables.Add(allInstrs[i]);
        }
        Variables.RemoveDuplicates();
    }

so that there is now a second PrintOpCode in addition to the one in Variables.Add, the program behaves correctly. I can't understand why adding a second PrintOpcode makes it work correctly. All print Opcode is is a function with a switch statement that just prints out a specific string depending on what the value of one of simple_instr's fields is.

VariableList Variables is contained inside of a separate struct called CFG

If you need more information/code i can provide it. If the answer is obvious I apologize, I don't program in C++ very often

EDIT:

One of the answers left, deleted now though, got me the fix.

Previously I was doing

static vector<Instruction*> ConvertLinkedListToVector(simple_instr* instructionList)
    {
        vector<Instruction*> convertedInstructions;
        int count = 0;
        for (simple_instr* current = instructionList; current; count++, current = current->next)
        {
            //Instruction* inst = new Instruction(*current);
            Instruction inst = Instruction(*current);
            inst.Id = count;
            convertedInstructions.push_back(&inst);
        }
        return convertedInstructions;
    }

to make the vector, but after reading that answer I changed it back to using "new" and it works correctly now. Thanks for the help, sorry for the dumb question heh

Most likely the const simple_instr& simple passed to your constructor goes out of scope, and you keep an invalid reference/pointer to a simple_instr.

Possibly not related your problem, but certainly a potential source of strange behaviour: Your Instruction(const simple_instr& simple) constructor may be getting called when you don't intend it. Mark it explicit...

explicit Instruction(const simple_instr& simple) ...

If that causes compiler errors, then that's progress :-) You might need to write a copy constructor to make them go away, and explicitly call the old constructor where you need to.

So, there are several suspicious observations:

• In your definition of VariableList you use a type called Variable - how is that type defined?
• Iterating over a container should be done using an iterator:

for (vector<Intruction *>::iterator it = allInstrs.begin(); 
     it != allInstrs.end();
     ++it) {
    Variables.Add(*it);
}

• You should consider using a vector of boost::shared_ptr, or a boost::ptr_vector instead of a vector of pointers.

I can give you a huge general overview of "don'ts" relating to your code.

• You are right in this case to use classes "deriving" from simple_instr but you are doing it wrong, given that later on you do a switch statement based on type. A switch-statement based on type (rather than state) is an anti-pattern. You should be calling some virtual method of your base class.

• You almost certainly do not want your derived class to copy from the base class. You want to construct it with the parameters to construct its base-class.

• You want a vector of the base class pointers? And to manage lifetime probably shared_ptr

• const-correctness. Some of your methods like size() should certainly be const. For others you might want two overloads.