stlFindInsert false positive when you only want to set it to an initial value

[Steve Albright]

In a loop with changing content (which seems critical to this and still could be ambiguous) and you want the insert only done if not in the container the first time, saying to skip the find is not always correct.

performance: stlFindInsert - Searching before insertion is not necessary.

void stlFindInsertFalsePositiveExample()
{
   std::map<int, std::string> stringList;
   std::string data = "DataString";

   for(int i=0; i<10; ++i)
   {
      data += "-OtherDataAppendedTenTimes";

      if(stringList.find(5) != stringList.end()) // this check doesn't let data keep growing
      {
         stringList[5] = data; // data keeps changing so the check above is valid
      }
   }
}

[CHR]

The code as stated above does nothing, since stringListis and remains empty. However, this avoids the duplicate lookup and silences the warning:

void stlFindInsertFalsePositiveExample()
{
   std::map<int, std::string> stringList;
   std::string data = "DataString";

   for(int i=0; i<10; ++i)
   {
      data += "-OtherDataAppendedTenTimes";

      auto it = stringList.find(5);
      if (it != stringList.end())
      {
         it->second = data;
      }
   }
}

https://godbolt.org/z/Gx1YGf1Kr

[Steve Albright]

I'm sorry, I had a typo above and I should have used == instead of !=

if(stringList.find(5) == stringList.end()) // this check doesn't let data keep growing

this way stringList does get populated but only once. Both however do produce the same warning.

What you are pointing out about the list always being empty seems like a different problem too that maybe should be caught by some other check?

[CHR]

Ok, in this case the warning can be avoided by using try_emplace() (available from C++17).

[Steve Albright]

Interesting but we are stuck with C++ 14 for quite a while to come but I'd rather have a better check since technically our code is doing what we want.

[Paul Fultz]

We do check for that the key is somewhat trivial in C++14, but we should probably check this on the value as well:

https://github.com/danmar/cppcheck/blob/main/lib/checkstl.cpp#L1571

[Daniel Marjamäki]

I don't immediately understand why we check that the key is complex. But do you think that matters in this case?

I have the feeling that we should not warn if the search is in a loop and the map variable is declared before the loop.

[Paul Fultz]

I don't immediately understand why we check that the key is complex. But do you think that matters in this case?

It was to fix this issue:

https://trac.cppcheck.net/ticket/9218

But it doesnt actually fix the issue entirely, since it only checks the value.

[Daniel Marjamäki]

I'd rather have a better check since technically our code is doing what we want.

Yes of course, it's a false positive. I have created https://trac.cppcheck.net/ticket/10558

[Daniel Marjamäki]

I have tried to clarify the error message. it will suggest to use map::insert or map::emplace instead. then the check is redundant.

[Daniel Marjamäki]

I'm sorry, I had a typo above and I should have used == instead of !=

that typo illustrates that it would be good to use insert/emplace. :-)