Is incrementing a map iterator in else part of ternary operator corect?

user_14078860 0
Viewed 123

Is code in Method 1 incorrect (using ternary operator). Does ++it has undefined evaluation order here. Or both methods are same:

Method 1:

struct X
{
    int id;
    int j;
};

void delete_data_from_map(map<int,X>& my_map, int const id)
{
    for(auto it {my_map.begin()}; it != my_map.end();)
    {
        it = (it->second.id == id) ? my_map.erase(it) : ++it;
    }
}

Method 2:

void delete_data_from_map(map<int,X>& my_map, int const id)
{
    for(auto it {my_map.begin()}; it != my_map.end();)
    {
        if(it->second.id == id)
             it = my_map.erase(it);
        else ++it;
    }
}
c++17 language-lawyer c++
3 Answers

There are three issues at play. One is the semantics of the ternary operator: only one of the possibilities of the conditional operator is evaluated. From [expr.cond]:

Only one of the second and third expressions is evaluated.

Further, you can rely that the chosen one will not start until the condition has fully ended:

Every value computation and side effect associated with the first expression is sequenced before every value computation and side effect associated with the second or third expression.

The second issue is the semantics of it = ++it;: a type may behave logically in a different way when assigned after an increment. Usually, we don't expect types to change the observable behavior, but we can't tell. It is best to write what you want: just the increment ++it;.

Finally, there is the topic of it = ++it; being UB pre-C++17 and whether you want to tie your code to that version.

Conclusion: it is best to write a plain if for this, precisely because the precise semantics and fear of UB is not worth it.

user_93053980

You do not provide enough context, therefore, I am guessing you want to search multiple items on some other attribute (not the key) and remove them.

If that assumption is correct that either approach is fine since you are incrementing the iterator to next element if the current one does not match your search condition.

Although, I would still recommend the if-else one because that is more clearer to the reader and also avoids any lingering sequence point issues the ternary version may have.

Deathstick_590810

Since it is of type map<int,X>::iterator, the expression

it = (it->second.id == id) ? my_map.erase(it) : ++it;

is well-defined in this case, because ++it is actually a function call; the update that happens inside the function is sequenced before the assignment expression.

user_68685430
Related