False positive (CWE: 398 -- can be declared as const) when there is a possibility of change

[Roy]

Code :

bool slave_scanning_set::remove_common(int const &panel, std::string &slave_removed_from,
    std::unordered_map<std::string, std::vector<int>> &map) {
  for (auto &[slave, panels] : map) {
    auto it = std::find(panels.begin(), panels.end(), panel);
    if (it != panels.end()) {
      panels.erase(it);
      slave_removed_from = slave;
      return true;
    }
  }
  return false;
}

map will be modified if panel is found on any of its values.

It should not be declared const.

cppcheck version : 2.4.1

[CHR]

I can't reproduce this when calling cppcheck.exe .\foo.cpp --enable=all --inconclusive on the code above. Maybe some more code is needed for the warning to be triggered?

[Roy]

Updated :

// foo.cpp
bool remove_common(int const &panel, std::string &slave_removed_from,
                   std::unordered_map<std::string, std::vector<int>> &map) {
  for (auto &[slave, panels] : map) {
    auto it = std::find(panels.begin(), panels.end(), panel);
    if (it != panels.end()) {
      panels.erase(it);
      slave_removed_from = slave;
      return true;
    }
  }
  return false;
}

bool remove(bool const &top, int const &panel, std::string &slave_removed_from,
            rocksdb::WriteBatch &batch) {
  std::lock_guard lk(mut_);
  bool ok;
  if (top) {
    ok = remove_common(panel, slave_removed_from, top_scanning_);
  } else {
    ok = remove_common(panel, slave_removed_from, bot_scanning_);
  }

  if (ok) {
    write_to_batch(batch);
  }
  return ok;
}

int main() {
  std::string slave;
  remove(true, 123, slave, batch);
}

Can repro with cppcheck.exe foo.cpp --enable=all --inconclusive

[CHR]

It seems the problem is related to C++17 structured binding. Slightly reduced example:

bool remove_common(int panel, std::unordered_map<std::string, std::vector<int>> &map) {
  for (auto &[slave, panels] : map) {
    auto it = std::find(panels.begin(), panels.end(), panel);
    if (it != panels.end()) {
      panels.erase(it);
      return true;
    }
  }
  return false;
}

This is clean:

bool remove_common2(int panel, std::unordered_map<std::string, std::vector<int>> &map) {
  for (auto &m : map) {
    auto it = std::find(m.second.begin(), m.second.end(), panel);
    if (it != m.second.end()) {
      m.second.erase(it);
      return true;
    }
  }
  return false;
}

Also, the warning disappears when a namespace is added to the function (slave_scanning_set:: in the OP)

[Daniel Marjamäki]

Thanks! I can reproduce. I created https://trac.cppcheck.net/ticket/10266