std::XXXX instead of raw loop

[AnotherUser]

Hi,
After upgrading to cppcheck 1.85, I'm getting a lot of style warnings for code similar to the below. The message is:

[test.cpp:13]: (style) Consider using std::transform algorithm instead of a raw loop.

vector<int> vec={1,3,5,6};
int var=3;
for(auto &v : vec)
{
    if(v > var)
            v+=var/2;
}

Often the altered code is not helpful, as it is larger and less readable than the original (perhaps there is a performance increase?). Discussions about the merits of std::transform here vs simple loops have been had elsewhere ( https://stackoverflow.com/questions/20028936/is-there-a-accummulate-if ).

I can disable or ignore this message myself, but perhaps default on is not so great, if the loop body has any complexity?

[Paul Fultz]

This comes from Sean Parent's Better Code talk. Raw loop suffer from many issues:

• Difficult to reason about and difficult to prove post conditions
• Error prone and likely to fail under non-obvious conditions
• Introduce non-obvious performance problems
• Complicates reasoning about the surrounding code

Even more so, Sean Parent suggests if you want to improve code quality than all coding guildines should be replaced with one guideline: “No raw loops”.

And in your snippet it does seem an algorithm would improve readbility. You have a loop which looks like you are accumulating a value, but you aren't doing an accumulate but a transform:

vector<int> vec={1,3,5,6};
int var=3;
std::tranform(vec.begin(), vec.end(), vec.begin(), [&](const auto& v) {
    return (v > var) ? var/2 : v;
});

This makes the intent of the code much clearer and it uses the same lines of code. It also clearer that this operation is associative since we know that std::transform is associative(this can be important if you want to extend this across varidiac number of inputs or use it in a parallel_reduce). Also, we can easily make the loop itself parallel in C++17 by adding std::execution::par to it.

I have personally used this guideline in my projects and I have found it to be quite helpful in improving readibility and extensibility. The only area where I have found to be kind of iffy is when doing an accumulate_tranform. There is no such algorithm and trying to do it directly can take away from the readability. I have found two options work well. Add a accumulate_transform function that wraps accumulate:

template<class Iterator, class State, class Transform, class Op>
State accumulate_transform(Iterator start, Iterator last, State state, Transform transform, Op op)
{
    return std::accumulate(start, last, state, [&](auto x, auto y) {
        return op(x, transform(y));
    });
}

Or use something like view::transform thats provided by most Range libraries such as Boost.Range, ranges-v3, etc.

[AnotherUser]

I think your example is incorrect. This returns: 1,3,1,1 for the vector.

This should be: 1,3,6,7.

To me this highlights the very danger with this code recommendation. Its quite hard to get the transform code right - you've made a simple mistake that is easy to make. This is one that cppcheck doesn't actually realise either. There is state in the loop that is kept, which is not being kept in your example.

I think the "no raw loops" is very subjective, and is not a fault per se - this is very much in opposition to the "no false positives" in cppcheck - here this is most definitely a false positive, due to the state that is kept in the loop.

I think this message does more harm than good.

[AnotherUser]

edit: the state comment is not right. My original example had state, but my posted one did not. I still think this is a good reason not to go back and change code that works for something that is not a clear improvement, and is conceptually tricky to get right.

[Daniel Marjamäki]

In general I think it's a bad idea to handwrite your own code instead of using ready made standard functions. So I thought this sounded very good at first. We have had some such warnings for years.

But well.. I have also got doubts about these particular warnings.

In my humble opinion, the first example raw loop is easier to read than the suggested std::transform code.

These warnings are suppressed in Cppcheck because nobody has cared to fix them. They are suppressed at my work also because I have not cared to fix them.

What do you think about skipping the warning if a lambda must be used? We can see if that is more acceptable.

[AnotherUser]

If it is possible to easily determine if a lambda is required, then that sounds like a really great solution!

[Paul Fultz]

To me this highlights the very danger with this code recommendation. Its quite hard to get the transform code right - you've made a simple mistake that is easy to make.

The tricky part was understanding the originial for loop, not understanding what std::tranform version does. If there was a mistake it was in my interpetation of the for loop not in writing std::transform.

I think the "no raw loops" is very subjective, and is not a fault per se - this is very much in opposition to the "no false positives" in cppcheck

So is reducing variable scope, but it is considered best practice, so cppcheck provides a check for both of this.

[Paul Fultz]

In my humble opinion, the first example raw loop is easier to read than the suggested std::transform code.

I actually find the std::transform easier to read and understand.

These warnings are suppressed in Cppcheck because nobody has cared to fix them. They are suppressed at my work also because I have not cared to fix them.

They are enabled on a couple of projects at my work. I have found the check to be quite useful. It has helped developers become more familiar with C++ algorithms. Developers have found more effecient ways to implement some things at times when they see the the relationship of operations in terms of algorithms, and we have found it much easier to extend some cases because of it being implemented in terms of algorithms.

One example from work is when computing numpy like broadcasting for two tensors, we rewrote our raw for loop as std::transform here:

https://github.com/ROCmSoftwarePlatform/AMDMIGraphX/blob/develop/src/onnx/onnx.cpp#L155

Of course, this only handles two tensors. When we needed to extended it to more than two tensors, it was much easier because it became obvious that this operation is associative(std::transform and std::max are associative). Therefore, we only need to use std::accumulate to extend it to multiple parameters:

https://github.com/ROCmSoftwarePlatform/AMDMIGraphX/blob/develop/src/onnx/onnx.cpp#L183

Such relationships can get lost when using raw for loops.

What do you think about skipping the warning if a lambda must be used?

What do you mean? A lambda is not necessary. I only used a lambda in this example for brevity and because I didnt know what the intention of the original code was. Obviously, for code that is more complicated, a function should be used. In the example above it could be written as:

int add_half_3_when_greater_than_3(int v) {
    return (v > var) ? v + var/2 : v;
}

void f() {
    vector<int> vec={1,3,5,6};
    std::tranform(
        vec.begin(), 
        vec.end(), 
        vec.begin(), 
        &add_half_3_when_greater_than_3));
}

Obviously, there could be a better name thanadd_half_3_when_greater_than_3 if I knew the intention of this code. With a proper name, this code can be much more readable than the raw for loop, as we cleary have a name for all the actions we are taking. Plus, it improves reusability, as the function is reusable for other point-wise operations. It also improves testability as the function can be tested standalone outside of the bigger operation.

Also, if there is some state associated with this, than a function class could be created instead.

[Paul Fultz]

My original example had state, but my posted one did not.

With internal state, it sounds like std::transform is not a good candidate. We could check that the variables used in assignment are not modified in the loop and skip warning about std::transform.

[Daniel Marjamäki]

I actually find the std::transform easier to read and understand.

ok. :-)

So is reducing variable scope, but it is considered best practice, so cppcheck provides a check for both of this.

That is a good example. Plenty of projects have to suppress that checker. It is common practice in C projects to declare the local variables at the start of the functions.

We should mostly avoid having such checkers that are "debatable" in Cppcheck. I don't want users to have to suppress "too much" checkers.

After your motivation I think we can keep this checker for now anyway in core Cppcheck. It sounds good that it was so useful for your team. Users that does not want it can suppress it for now.

What do you mean? A lambda is not necessary.

I actually meant to only write the warning if the loop is just using a standard function or something. Like a loop that lowercases a string using std::tolower().

Maybe those warnings would teach the users about the algorithms anyway and then they could judge if they want to use the algorithms in more complex situations.

[Daniel Marjamäki]

I actually meant to only write the warning if the loop is just using a standard function or something. Like a loop that lowercases a string using std::tolower().

I think you can make the decision about this. If you want to have the checker this way then we will try that.

[versat]

The message for reducing variable scope contains a warning that one has to be careful to not introduce a bug by reducing the variable too much or so.
Does the message suggesting an algorithm contain a similar warning? Would it make sense to warn here too?

[Kazutoshi Satoda]

It looks bad to use std::transform() to replace a raw loop where the
transformation (assignment in general?) is conditional like the example
in OP; it causes unnecessary loads and stores and may lead to a
performance issue especially if the condition is rarely taken.

I also think the raw loops tend to be simpler where the assignment is
made on the original range.

[Paul Fultz]

It looks bad to use std::transform() to replace a raw loop where the
transformation (assignment in general?) is conditional like the example
in OP; it causes unnecessary loads and stores and may lead to a
performance issue especially if the condition is rarely taken.

This is a good point. We shouldn't suggest std::transform. If std::transform_if is added in a future version of C++, we could suggest std::transform_if instead:

http://open-std.org/JTC1/SC22/WG21/docs/papers/2017/p0838r0.pdf

[Daniel Marjamäki]

I think it sounds like you plan to keep the checker as it is. I respect that. Could you please use separate IDs then?

One ID for simple cases where the replacement does not require lambda or new function.

One ID for all the other cases that you currently warn about.