C++ CppCheck algorithm suggestion (std::find_if instead of raw loop) pertinence

Question

CppCheck suggest me to replace one of my code by a STL algorithm, I'm not against it, but I don't know how to replace it. I'm pretty sure this is a bad suggestion (There is warning about experimental functionalities in CppCheck).

Here is the code :

    /* Cutted beginning of the function ... */

    for ( const auto & program : m_programs )
    {
        if ( program->compare(vertexShader, tesselationControlShader, tesselationEvaluationShader, geometryShader, fragmentShader) )
        {
            TraceInfo(Classname, "A program has been found matching every shaders.");

            return program;
        }
    }

    return nullptr;
} /* End of the function */

And near the if condition I got : "Consider using std::find_if algorithm instead of a raw loop."

I tried to use it, but I can't get the return working anymore... Should I ignore this suggestion ?

Sergey Kolesnik · Accepted Answer · 2019-12-01 14:09:56Z

2

I suppose you may need to use that finding function not once. So, according to DRY, you need to separate the block where you invoke an std::find_if algorithm to a distinct wrapper function.

{

    // ... function beginning

    auto found = std::find_if(m_programs.cbegin(), m_programs.cend(), 
       [&](const auto& prog)
       {
           bool b = prog->compare(...);
           if (b)
                 TraceInfo(...);
           return b;
       });

    if (found == m_programs.cend())
       return nullptr;

    return *found;

}

The suggestion is good. An STL algorithm migth be able to choose an appropriate approach based on your container type.

Furthermore, I suggest you to use a self-balancing container like an std::set.


// I don't know what kind of a pointer you use. 
using pProgType = std::shared_pointer<ProgType>;

bool compare_progs(const pProgType &a, const pProgType &b)
{
   return std::less(*a, *b);
}

std::set<std::shared_pointer<prog_type>, 
    std::integral_constant<decltype(&compare_progs), &compare_progs>> progs.

This is a sorted container, so you will spend less time for searching a program by a value, given you implement a compare operator (which is invoked by std::less).

If you can use an stl function, use it. This way you will not have to remember what you invented, because stl is properly documented and safe to use.

edited Dec 1, 2019 at 14:09

answered Dec 1, 2019 at 13:30

Sergey Kolesnik

4,0182 gold badges15 silver badges48 bronze badges

Sign up to request clarification or add additional context in comments.

6 Comments

Sébastien Bémelmans Over a year ago

Ok, I got the idea now... Its working ! But, for a stylistic point of view, it's a bit darker no ?

Sébastien Bémelmans Over a year ago

I see what you want me to improve with my compare function (which is not correctly named as it appears now). In fact, I can't really compare two program in that place, because the new program isn't existing yet. I'm just watching against living programs if I really need to build a new flavor of a program based on shaders footprint. But I love the idea, noted ! :)

Sergey Kolesnik Over a year ago

@SébastienBémelmans you are better of using std::set::insert then. It would check if the key exists and then emplace an instance. You will need to implement an operator== then. Also, check out en.cppreference.com/w/cpp/container/set

Sébastien Bémelmans Over a year ago

So if I follow your idea, I will change my class member "std::vector< std::shared_ptr< Program > > m_programs;" by "std::set::< std::shared_ptr< Program > > m_programs;" ?

Sergey Kolesnik Over a year ago

Well, yes. But don't forget the second argument, which is a compare function, wrapped with std::integral_constant. That will enable the container to compare objects, not pointers. Though I'm not sure that your approach in general is good, given you have to search if there is a shader program with same parameters. I would avoid the possibility of just having two similar programs rather than run that kind of check.

|

Collectives™ on Stack Overflow

C++ CppCheck algorithm suggestion (std::find_if instead of raw loop) pertinence

1 Answer 1

6 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

6 Comments

Your Answer

Sign up or log in

Post as a guest

Related