cpp:S6165 Should not suggest when altering data

Clean Code Report False-positive / False-negative...
1

Given to following code:

auto i = begin(entries);
while (i != end(entries))
{
    if (i->second.life == 0)
    {
        i = entries.erase(i);
    }
    else
    {
        i->second.life = i->second.life - 1;
        ++i;
    }
}

I get a cpp:S6165 flagged and recommended to use std::erase_if.

I don’t think this is a good suggestion, because either I use something like

std::ranges::for_each(entries, [] (auto& entry) {
    entry.second.life -= 1;
});
std::erase_if(entries, [] (const auto& entry) {
    return entry.second.life == 0;
});

Which means I need to iterate over the collection twice.

Or

std::erase_if(entries, [] (auto& entry) {
    if (entry.second.life == 0)
    {
        return true;
    }
    entry.second.life -= 1;
    return false;
});

Which works, but IMHO is not the way erase_if is suposed to be used and is a code smell in it’s own right.

5

Hello @rioki, and welcome to our community!

Indeed, this seems to be a false positive. I’ve raised [CPP-5468] - Jira to track the issue.
Thanks for your feedback.

I mostly agree with your assessment. Regarding having two “loops” (for_each & erase_if), I would note two things:

  1. The order of the calls should be reversed to preserve the current behavior. I.e., call std::erase_if first, then decrease the life. Otherwise, you get life == -1 and not 0.
  2. I would still advocate for this solution if performance is not a concern because the code is easier to read (there are two clearly distinct actions, one that reduces the “life” and the other that evicts “dead” entries from the container). If the code is in the hot path, then your argument definitely stands. However, it can be surprisingly deceptive how marginal the difference can be.

Finally, I definitely agree that a mutating predicate is bad practice. This is not the case for std::erase_if, but some other STL algorithms expect the predicate not to mutate the input. It, therefore, makes sense to apply the same rule to std::erase_if.

Cheers