Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 3 commits into from
Nov 16, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e1d9f2a
Fix modernize-loop-convert warning reported by clang-tidy
MaximSmolskiy Nov 13, 2022
21d592a
Merge branch 'unstable' into fix-modernize-loop-convert-warning-repor…
MaximSmolskiy Nov 15, 2022
4332c3a
Remove comment
MaximSmolskiy Nov 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .clang-tidy
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# refer to https://clang.llvm.org/extra/clang-tidy/checks/list.html
Checks: -*, clang-analyzer-core.*, clang-analyzer-cplusplus.*, clang-analyzer-deadcode.*, clang-analyzer-nullability.*, clang-analyzer-security.*, clang-analyzer-unix.*, clang-analyzer-valist.*, cppcoreguidelines-init-variables, cppcoreguidelines-macro-usage, cppcoreguidelines-interfaces-global-init, cppcoreguidelines-narrowing-conversions, cppcoreguidelines-no-malloc, cppcoreguidelines-prefer-member-initializer, cppcoreguidelines-special-member-functions, cppcoreguidelines-slicing, google-build-explicit-make-pair, google-default-arguments, google-explicit-constructor, modernize-avoid-bind, modernize-loop-convert, modernize-macro-to-enum, modernize-make-shared, modernize-make-unique, modernize-pass-by-value, modernize-redundant-void-arg, modernize-return-braced-init-list, modernize-use-auto, modernize-use-bool-literals, modernize-use-emplace, modernize-use-equals-default, modernize-use-equals-delete, modernize-use-nullptr, modernize-use-override, modernize-use-using, performance-faster-string-find, performance-for-range-copy, performance-implicit-conversion-in-loop, performance-inefficient-algorithm, performance-inefficient-vector-operation, performance-move-const-arg, performance-move-constructor-init, performance-no-automatic-move, performance-trivially-destructible, performance-type-promotion-in-math-fn, performance-unnecessary-copy-initialization, performance-unnecessary-value-param

WarningsAsErrors: clang-analyzer-*, -clang-analyzer-security.insecureAPI.rand, cppcoreguidelines-interfaces-global-init, cppcoreguidelines-no-malloc, cppcoreguidelines-slicing, google-*, modernize-use-emplace, modernize-use-equals-default, modernize-use-equals-delete, performance-implicit-conversion-in-loop, performance-inefficient-algorithm, performance-move-constructor-init, performance-no-automatic-move, performance-trivially-destructible, performance-type-promotion-in-math-fn, performance-unnecessary-copy-initialization, modernize-use-bool-literals, performance-unnecessary-value-param, modernize-make-unique, performance-for-range-copy, performance-faster-string-find, modernize-redundant-void-arg, modernize-avoid-bind, modernize-use-auto, modernize-use-using, performance-inefficient-vector-operation, cppcoreguidelines-special-member-functions
WarningsAsErrors: clang-analyzer-*, -clang-analyzer-security.insecureAPI.rand, cppcoreguidelines-interfaces-global-init, cppcoreguidelines-no-malloc, cppcoreguidelines-slicing, google-*, modernize-use-emplace, modernize-use-equals-default, modernize-use-equals-delete, performance-implicit-conversion-in-loop, performance-inefficient-algorithm, performance-move-constructor-init, performance-no-automatic-move, performance-trivially-destructible, performance-type-promotion-in-math-fn, performance-unnecessary-copy-initialization, modernize-use-bool-literals, performance-unnecessary-value-param, modernize-make-unique, performance-for-range-copy, performance-faster-string-find, modernize-redundant-void-arg, modernize-avoid-bind, modernize-use-auto, modernize-use-using, performance-inefficient-vector-operation, cppcoreguidelines-special-member-functions, modernize-loop-convert

CheckOptions:
- key: cppcoreguidelines-special-member-functions.AllowSoleDefaultDtor
Expand Down
4 changes: 2 additions & 2 deletions src/common/task_runner.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,8 @@ void TaskRunner::Stop() {
}

void TaskRunner::Join() {
for (size_t i = 0; i < threads_.size(); i++) {
if (threads_[i].joinable()) threads_[i].join();
for (auto &thread : threads_) {
if (thread.joinable()) thread.join();
}
}

Expand Down
6 changes: 3 additions & 3 deletions src/stats/stats.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,8 @@ Stats::Stats() {
im.last_sample_time = 0;
im.last_sample_count = 0;
im.idx = 0;
for (int j = 0; j < STATS_METRIC_SAMPLES; j++) {
im.samples[j] = 0;
for (uint64_t &sample : im.samples) {
sample = 0;
}
inst_metrics.push_back(im);
}
Expand Down Expand Up @@ -105,6 +105,6 @@ void Stats::TrackInstantaneousMetric(int metric, uint64_t current_reading) {

uint64_t Stats::GetInstantaneousMetric(int metric) {
uint64_t sum = 0;
for (int j = 0; j < STATS_METRIC_SAMPLES; j++) sum += inst_metrics[metric].samples[j];
for (uint64_t sample : inst_metrics[metric].samples) sum += sample;
return sum / STATS_METRIC_SAMPLES;
}
4 changes: 2 additions & 2 deletions src/storage/redis_db.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -611,8 +611,8 @@ std::vector<std::string> *WriteBatchLogData::GetArguments() { return &args_; }

std::string WriteBatchLogData::Encode() {
std::string ret = std::to_string(type_);
for (size_t i = 0; i < args_.size(); i++) {
ret += " " + args_[i];
for (const auto &arg : args_) {
ret += " " + arg;
}
return ret;
}
Expand Down
4 changes: 2 additions & 2 deletions src/types/redis_set.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,8 +308,8 @@ rocksdb::Status Set::Union(const std::vector<Slice> &keys, std::vector<std::stri

std::map<std::string, bool> union_members;
std::vector<std::string> target_members;
for (size_t i = 0; i < keys.size(); i++) {
auto s = Members(keys[i], &target_members);
for (const auto &key : keys) {
auto s = Members(key, &target_members);
if (!s.ok()) return s;
for (const auto &member : target_members) {
union_members[member] = true;
Expand Down
7 changes: 2 additions & 5 deletions src/types/redis_string.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,11 +134,8 @@ std::vector<rocksdb::Status> String::MGet(const std::vector<Slice> &keys, std::v
}
std::vector<Slice> slice_keys;
slice_keys.reserve(ns_keys.size());
// don't use range-based for loop here, coz the slice member
// would refer the address instead of copy the value, and use
// range-based for loop may cause all members refer to the same addr
for (size_t i = 0; i < ns_keys.size(); i++) {
slice_keys.emplace_back(ns_keys[i]);
for (const auto &ns_key : ns_keys) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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());

Copy link
Member

@git-hulk git-hulk Nov 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

slice_keys.emplace_back(ns_key);
}
return getValues(slice_keys, values);
}
Expand Down