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.

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

Fix performance-inefficient-vector-operation warning reported by clang-tidy #1104

Closed
1 of 2 tasks
Tracked by #1029
PragmaTwice opened this issue Nov 12, 2022 · 0 comments · Fixed by #1115
Closed
1 of 2 tasks
Tracked by #1029

Fix performance-inefficient-vector-operation warning reported by clang-tidy #1104

PragmaTwice opened this issue Nov 12, 2022 · 0 comments · Fixed by #1115
Labels
enhancement type enhancement good first issue Good for newcomers

Comments

@PragmaTwice
Copy link
Member

Search before asking

  • I had searched in the issues and found no similar issues.

Motivation

This issue is tracked by #1029.

We welcome new contributors to take these issues as a beginning of a deep dive to kvrocks

Currently we have enabled lots of clang-tidy checks, but there are so many reports already exist in the kvrocks code, so we cannot treat these report as errors to block future PR with some clang-tidy reported warnings in CI.

Hence the goal of this issue is to solve all performance-inefficient-vector-operation tagged clang-tidy reports, and then enable warnings-as-errors for this specific check in .clang-tidy.

To get clang-tidy reports for latest kvrocks code, there are several ways: you can run ./x.py check tidy locally, or check the log of the latest run of GitHub Actions on the unstable branch (e.g. https://github.com/apache/incubator-kvrocks/actions/runs/3449328741/jobs/5757148924#step:8:916). To be friendly for new contributors, we list all performance-inefficient-vector-operation tagged reports below, so in normal cases you can just follow the log below and fix them one by one.

/home/runner/work/incubator-kvrocks/incubator-kvrocks/src/types/redis_string.cc:402:5: warning: 'emplace_back' is called inside a loop; consider pre-allocating the container capacity before the loop [performance-inefficient-vector-operation]
    keys.emplace_back(pair.key);
    ^
/home/runner/work/incubator-kvrocks/incubator-kvrocks/src/types/redis_zset.cc:755:7: warning: 'emplace_back' is called inside a loop; consider pre-allocating the container capacity before the loop [performance-inefficient-vector-operation]
      mscores.emplace_back(MemberScore{iter.first, iter.second});
      ^
/home/runner/work/incubator-kvrocks/incubator-kvrocks/src/commands/redis_cmd.cc:1462:7: warning: 'emplace_back' is called inside a loop; consider pre-allocating the container capacity before the loop [performance-inefficient-vector-operation]
      keys.emplace_back(fv.field);
      ^
/home/runner/work/incubator-kvrocks/incubator-kvrocks/src/commands/redis_cmd.cc:1480:7: warning: 'emplace_back' is called inside a loop; consider pre-allocating the container capacity before the loop [performance-inefficient-vector-operation]
      values.emplace_back(p.value);
      ^
/home/runner/work/incubator-kvrocks/incubator-kvrocks/src/commands/redis_cmd.cc:4279:11: warning: 'emplace_back' is called inside a loop; consider pre-allocating the container capacity before the loop [performance-inefficient-vector-operation]
          keys.emplace_back(args_[key_index + 2]);
          ^
/home/runner/work/incubator-kvrocks/incubator-kvrocks/src/commands/redis_cmd.cc:4562:7: warning: 'emplace_back' is called inside a loop; consider pre-allocating the container capacity before the loop [performance-inefficient-vector-operation]
      score_strings.emplace_back(Util::Float2String(score));
      ^

Some of these reports will give modification suggestions in the log, you can refer to them directly; otherwise, you need to understand the content of the warning and combine your C++ knowledge to fix it. (Fortunately, most reports (except those by clang static analyzer) are intuitive and easy to fix.)

When you fixed these all reports listed above, you can now add the check performance-inefficient-vector-operation to warnings-as-errors in .clang-tidy (in the root dir of the repo): We show a sample diff below to illustrate how to modify .clang-tidy:

--- a/.clang-tidy
+++ b/.clang-tidy
@@ -1,7 +1,7 @@
 # refer to https://clang.llvm.org/extra/clang-tidy/checks/list.html
 Checks: ...

-WarningsAsErrors: ...
+WarningsAsErrors: ..., performance-inefficient-vector-operation

Then, if all of the above are completed, congratulations, you can submit a PR for your changes now!

Solution

No response

Are you willing to submit a PR?

  • I'm willing to submit a PR!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement type enhancement good first issue Good for newcomers
Projects
None yet
1 participant