Dysfunctionality of the iterator

Question

Dysfunctionality of the iterator

I have some code where I basically have positive values and negative values. Negative values are operators ('+' = - 1, '-' = - 2, '*' = - 3, '/' = - 4) and I basically have to either divide to make the sum and so on 2 numbers preceding a negative value.

std::list<long>::iterator i,j;
for(i=num.begin();++i!=num.end();)
{
            if(*i<0&&num.size()>=2)
            {
                    if(*i==-1)
                    {
                              *i=*--(j=i)+*----(j=i);
                    }
                    else if(*i==-2)
                    {
                              *i=*----(j=i)-*--(j=i);
                    }
                    else if(*i==-3)
                    {
                              *i=*--(j=i)**----(j=i);
                    }
                    else if(*i==-4&&*--(j=i)!=0)
                    {
                              *i=*----(j=i)/(*--(j=i));
                    }//this part is working properly
                    num.erase(--(j=i));
                    num.erase(--(j=i));//here is the only problem
                    break;
    }
}

Apparently I'm trying to remove a value from a list that doesn't exist.

-2

c ++ iterator list

Mihnea gafton 11 nov. 14 at 11:58

source to share

3 answers

Jarod42 · Answer 1 · 2014-11-11T12:06:11+0000

You have undefined behavior with:

*--(j=i) + *----(j=i);

where you change multiple times j

in an unspecified order. using std::prev

will solve this and make the code cleaner.

Barry · Answer 2 · 2014-11-11T12:06:52+0000

Departing from the fact that some of your codes are undefined behavior and are completely unnecessarily cryptic.

std::list<long>::iterator i,j;
for(i=num.begin();++i!=num.end();)
{
    // ...
    num.erase(std::prev(i));
    num.erase(std::prev(i));
}

We know that num.size()

> = 2, but we don't know that there i

are at least 2 past beginnings, so there are actually two things to erase. It is likely that your first and / or second is running through the loop, trying to erase non-existent iterators.

[edit] Apparently your loop check ++i != num.end()

. First, don't do this. Second, I assume this effectively means that you start one initial start, so why are you failing at the second erasure in the first iteration of the loop:

[begin] <--> [item] <--> [item] <--> ...
             ^
             i

You are trying to erase two elements before i

. There's only one here.

James kanze · Answer 3 · 2014-11-11T14:07:14+0000

There are serious problems in your code:

In type expressions, the *--(j = i) + *----(j = i)

compiler is free to reorder operators in almost any way that does not violate direct dependencies. Thus, it can do two assignments first, then three decrements, and finally two differences (resulting in something equivalent *(i - 3) + *(i - 3)

). Or it could do an assignment and two reductions on the right, then assignment and decrement on the left, and then two dilutions, resulting in an equivalent *(i - 1) + *(i - 1)

. Or any other combination. (If i

u j

were pointers and operators were built-in types, you would have undefined behavior.) Generally: no expression should have more than one side effect. Not just because of the order, but because you want people to be able to read your code. (This comment also applies to things like++i != num.end()

; except in a few cases, condition a for

, while

or if

shouldn't have side effects.)
Erasing the elements of a container while iterating over it should be avoided unless you really know what you are doing. Depending on the container type, this may invalidate your iterators; in the case of std::list

, it shouldn't until the element being erased is the one denoted by the iterator, but it's very difficult to figure out where your iterator ends. In your case, you do not seem to take any precautions for the case where i

the second item is pointed; if you try to erase the element in front of it twice, you will be in trouble.

Finally, it seems to me that you are trying to evaluate an expression in RPN. I will convey the fact that you use negative values to specify operators - the best solution would be a structure with a field for the operator and a field for the value (ignored in the case of type operators '+'

). Something like:

enum Operator
{
    push,
    add,
    sub,
    mult,
    div
};

struct Term
{
    Operator op;
    long value;
};

But the classic way of doing this would be a value stack: iterating over an expression without mutating it, pushing values onto the stack and popping, performing an operation, and pushing results for statements. If the expression is valid, you end up with one item on the stack: check it out, and that there are at least enough items for the operators.

And don't forget that there is such a thing as a switch. Your loop should be something like:

std::stack<long> stack;
for ( auto current = num.cbegin(), end = num.cend(); current != end; ++ current ) {
    switch ( current->op ) {
    case push:
        stack.push( current->value );
        break;
    //  ...
    }
}
if ( stack.size() != 1 ) {
    //  error...
} else {
    //  results are the only remaining element on the stack.
}

Dysfunctionality of the iterator

More articles: