Fix modernize-loop-convert warning reported by clang-tidy

[MaximSmolskiy]

Fix #1095

[torwig]

@MaximSmolskiy This place is really tricky. The comment above it explains why we should NOT use a range-based loop here.

[MaximSmolskiy]

@torwig Can you please explain this comment in more details?
ns_keys is std::vector<std::string>. std::vector operator[](size_type pos) always returns reference (as said here), so ns_keys[i] was reference to std::string. And I don't understand how range-based for loop can cause problems here, because ns_key now is reference to std::string too (as it was).

[PragmaTwice]

emplace_back will invalidate all references and iterators (if capacity changes), so it is not allowed to use an iterator or reference based loop (as well as for-range) here. Of cource, index is always safe.

[MaximSmolskiy]

@PragmaTwice I'm sorry, but I still don't understand what the problem is. Here we iterate through vector ns_keys and put its values to another vector slice_keys. So, ns_keys shouldn't be updated during this loop. So, range-based for loop should be equivalent to index based loop here.

[PragmaTwice]

Oh sorry. I did not notice that there are two different vectors. Some time to wait me to understand these code again 🤣

[PragmaTwice]

Actually I cannot understand the comment. Slice will never copy a string, it will only take reference to a string. cc @git-hulk @ShooterIT

[torwig]

I've just tested the following code:

std::vector<std::string> ns_keys = {"one", "two", "three", "four"};
std::vector<rocksdb::Slice> slice_keys;
for (size_t i = 0; i < ns_keys.size(); i++) {
  slice_keys.emplace_back(ns_keys[i]);
}

for (auto key : slice_keys) {
  std::cout << key.ToString() << std::endl;
}

And the output is:

one
two
three
four

Then I changed it to (as in the clang recommendation):

std::vector<std::string> ns_keys = {"one", "two", "three", "four"};
std::vector<rocksdb::Slice> slice_keys;
for (auto &ns_key : ns_keys) {
  slice_keys.emplace_back(ns_key);
}

for (auto key : slice_keys) {
  std::cout << key.ToString() << std::endl;
}

And the output is still the same.
Perhaps at some point in time, the code in kvrocks was changed but the comment is still here (an doutdated). Or there is something else.

[PragmaTwice]

I think it can be something like:

std::vector<rocksdb::Slice> slice_keys(ns_keys.size());
std::copy(ns_keys.begin(), ns_keys.end(), slice_keys.begin());

[git-hulk]

@PragmaTwice @torwig @MaximSmolskiy If my memory serves, the root cause is the for loop would reuse the same variable ns_key address if we use for (auto ns_key : ns_keys) instead of for (auto &ns_key : ns_keys).

So we can remove this comment if for (auto &ns_key : ns_keys) works well.

[PragmaTwice]

I think we can still use range-based for loop and remove these comment to continue with this PR if it is confirmed that these comments are out-of-date and not correct now.
cc @git-hulk

[PragmaTwice]

Thanks for your contribution!