Use clang-tidy instead of cppcheck (#1025)

Co-authored-by: tison <wander4096@gmail.com>

.clang-tidy

@@ -0,0 +1,12 @@
+# 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
+
+CheckOptions:
+  - key:           cppcoreguidelines-special-member-functions.AllowSoleDefaultDtor
+    value:         True
+  - key:           cppcoreguidelines-special-member-functions.AllowMissingMoveFunctionsWhenCopyIsDeleted
+    value:         True
+  - key:           performance-move-const-arg.CheckTriviallyCopyableMove
+    value:         False

.github/workflows/kvrocks.yaml

@@ -45,8 +45,7 @@ jobs:
       - name: Install Check Tools
         run: |
           sudo apt update
-          sudo apt install -y cppcheck
-          sudo apt install -y clang-format-12
+          sudo apt install -y clang-format-12 clang-tidy-12
       - name: Setup Go
         uses: actions/setup-go@v3
         with:
@@ -65,10 +64,12 @@ jobs:
         if: always() && steps.check-format.outcome != 'success'
         with:
           path: clang-format.patch
-      - name: Lint and check code
+      - name: Lint gocase code
+        run: ./x.py check golangci-lint
+      - name: Check cpp code via clang-tidy
         run: |
-          ./x.py check cppcheck
-          ./x.py check golangci-lint
+          ./x.py build --skip-build
+          ./x.py check tidy -j $(nproc) --clang-tidy-path clang-tidy-12 --run-clang-tidy-path run-clang-tidy-12
 
   build-and-test:
     name: Build and test

src/commands/redis_cmd.cc

@@ -584,7 +584,7 @@ class CommandSet : public Commander {
     return Commander::Parse(args);
   }
   Status Execute(Server *svr, Connection *conn, std::string *output) override {
-    int ret;
+    int ret = 0;
     Redis::String string_db(svr->storage_, conn->GetNamespace());
     rocksdb::Status s;
 

src/common/event_util.h

@@ -29,7 +29,7 @@
 template <typename F, F *f>
 struct StaticFunction {
   template <typename... Ts>
-  auto operator()(Ts &&...args) const -> decltype(f(std::forward<Ts>(args)...)) {
+  auto operator()(Ts &&...args) const -> decltype(f(std::forward<Ts>(args)...)) {  // NOLINT
     return f(std::forward<Ts>(args)...);
   }
 };
@@ -44,7 +44,6 @@ struct UniqueFreePtr : std::unique_ptr<T, StaticFree> {
 };
 
 struct UniqueEvbufReadln : UniqueFreePtr<char[]> {
-  // cppcheck-suppress uninitMemberVar
   UniqueEvbufReadln(evbuffer *buffer, evbuffer_eol_style eol_style)
       : UniqueFreePtr(evbuffer_readln(buffer, &length, eol_style)) {}
 

src/common/sha1.cc

@@ -188,7 +188,7 @@ void SHA1Transform(uint32_t state[5], const unsigned char buffer[64]) {
   state[3] += d;
   state[4] += e;
   /* Wipe variables */
-  a = b = c = d = e = 0;
+  a = b = c = d = e = 0;  // NOLINT
 #ifdef SHA1HANDSOFF
   memset(block, '\0', sizeof(block));
 #endif

src/common/util.cc

@@ -637,9 +637,9 @@ std::vector<std::string> TokenizeRedisProtocol(const std::string &value) {
 }
 
 bool IsPortInUse(int port) {
-  int fd;
+  int fd = NullFD;
   Status s = SockConnect("0.0.0.0", static_cast<uint32_t>(port), &fd);
-  if (fd > 0) close(fd);
+  if (fd != NullFD) close(fd);
   return s.IsOK();
 }
 

src/config/config_util.cc

@@ -137,10 +137,8 @@ StatusOr<ConfigKV> ParseConfigLine(const std::string& line) {
 
   if (state == KEY) {
     res.first = current_str;
-    state = AFTER_KEY_SPACE;
   } else if (state == NORMAL) {
     res.second = Util::Trim(current_str, " \t\r\n\v\f\b");
-    state = AFTER_VAL_SPACE;
   } else if (state == QUOTED || state == ESCAPE) {
     return {Status::NotOK, "config line ends unexpectedly in quoted string"};
   } else if (state == ERROR) {

src/server/worker.cc

@@ -101,7 +101,7 @@ void Worker::TimerCB(int, int16_t events, void *ctx) {
 
 void Worker::newTCPConnection(evconnlistener *listener, evutil_socket_t fd, sockaddr *address, int socklen, void *ctx) {
   auto worker = static_cast<Worker *>(ctx);
-  int local_port = Util::GetLocalPort(fd);
+  int local_port = Util::GetLocalPort(fd);  // NOLINT
   DLOG(INFO) << "[worker] New connection: fd=" << fd << " from port: " << local_port << " thread #" << worker->tid_;
   auto s = Util::SockSetTcpKeepalive(fd, 120);
   if (!s.IsOK()) {

src/types/geohash.cc

@@ -261,7 +261,7 @@ static void geohash_move_x(GeoHashBits *hash, int8_t d) {
   uint64_t x = hash->bits & 0xaaaaaaaaaaaaaaaaULL;
   uint64_t y = hash->bits & 0x5555555555555555ULL;
 
-  uint64_t zz = 0x5555555555555555ULL >> (64 - hash->step * 2);
+  uint64_t zz = 0x5555555555555555ULL >> (64 - hash->step * 2);  // NOLINT
 
   if (d > 0) {
     x = x + (zz + 1);
@@ -270,7 +270,7 @@ static void geohash_move_x(GeoHashBits *hash, int8_t d) {
     x = x - (zz + 1);
   }
 
-  x &= (0xaaaaaaaaaaaaaaaaULL >> (64 - hash->step * 2));
+  x &= (0xaaaaaaaaaaaaaaaaULL >> (64 - hash->step * 2));  // NOLINT
   hash->bits = (x | y);
 }
 

src/types/geohash.h

@@ -88,13 +88,13 @@ typedef enum {
 } GeoDirection;
 
 typedef struct {
-  uint64_t bits;
-  uint8_t step;
+  uint64_t bits = 0;
+  uint8_t step = 0;
 } GeoHashBits;
 
 typedef struct {
-  double min;
-  double max;
+  double min = 0;
+  double max = 0;
 } GeoHashRange;
 
 typedef struct {

src/types/redis_zset.cc

@@ -476,7 +476,7 @@ rocksdb::Status ZSet::RangeByLex(const Slice &user_key, ZRangeLexSpec spec, std:
     if (spec.count > 0 && members && members->size() >= static_cast<unsigned>(spec.count)) break;
   }
 
-  if (spec.removed && *size > 0) {
+  if (spec.removed && size && *size > 0) {
     metadata.size -= *size;
     std::string bytes;
     metadata.Encode(&bytes);

x.py

@@ -30,6 +30,7 @@
 
 CMAKE_REQUIRE_VERSION = (3, 16, 0)
 CLANG_FORMAT_REQUIRED_VERSION = (12, 0, 0)
+CLANG_TIDY_REQUIRED_VERSION = (12, 0, 0)
 
 SEMVER_REGEX = re.compile(
     r"""
@@ -86,7 +87,7 @@ def check_version(current: str, required: Tuple[int, int, int], prog_name: Optio
 
     return semver
 
-def build(dir: str, jobs: int, ghproxy: bool, ninja: bool, unittest: bool, compiler: str, cmake_path: str, D: List[str]) -> None:
+def build(dir: str, jobs: int, ghproxy: bool, ninja: bool, unittest: bool, compiler: str, cmake_path: str, D: List[str], skip_build: bool) -> None:
     basedir = Path(__file__).parent.absolute()
 
     find_command("autoconf", msg="autoconf is required to build jemalloc")
@@ -113,6 +114,9 @@ def build(dir: str, jobs: int, ghproxy: bool, ninja: bool, unittest: bool, compi
         cmake_options += [f"-D{o}" for o in D]
     run(cmake, str(basedir), *cmake_options, verbose=True, cwd=dir)
 
+    if skip_build:
+        return
+
     target = ["kvrocks", "kvrocks2redis"]
     if unittest:
         target.append("unittest")
@@ -152,22 +156,24 @@ def clang_format(clang_format_path: str, fix: bool = False) -> None:
 
     run(command, *options, *sources, verbose=True, cwd=basedir)
 
-def cppcheck() -> None:
-    command = find_command("cppcheck", msg="cppcheck is required")
-    basedir = Path(__file__).parent.absolute()
+def clang_tidy(dir: str, jobs: int, clang_tidy_path: str, run_clang_tidy_path: str) -> None:
+    # use the run-clang-tidy Python script provided by LLVM Clang
+    run_command = find_command(run_clang_tidy_path, msg="run-clang-tidy is required")
+    tidy_command = find_command(clang_tidy_path, msg="clang-tidy is required")
 
-    options = ["-x", "c++"]
-    options.append("-U__GNUC__")
-    options.append("--force")
-    options.append("--std=c++11")
-    # we should run cmake configuration to fetch deps if we want to enable missingInclude
-    options.append("--enable=warning,portability,information")
-    options.append("--error-exitcode=1")
-    options.append("--inline-suppr")
+    version_res = run_pipe(tidy_command, '--version').read().strip()
+    version_str = re.search(r'version\s+((?:\w|\.)+)', version_res).group(1)
 
-    sources = ["src"]
+    check_version(version_str, CLANG_TIDY_REQUIRED_VERSION, "clang-tidy")
 
-    run(command, *options, *sources, verbose=True, cwd=basedir)
+    if not (Path(dir) / 'compile_commands.json').exists():
+        raise RuntimeError(f"expect compile_commands.json in build directory {dir}")
+
+    basedir = Path(__file__).parent.absolute()
+
+    options = ['-p', dir, '-clang-tidy-binary', tidy_command, f'-j{jobs}']
+
+    run(run_command, *options, 'kvrocks/src/', verbose=True, cwd=basedir)
 
 def golangci_lint() -> None:
     go = find_command('go', msg='go is required for testing')
@@ -293,13 +299,17 @@ def test_go(dir: str, rest: List[str]) -> None:
         help="Check source format by clang-format")
     parser_check_format.set_defaults(func=lambda **args: clang_format(**args, fix=False))
     parser_check_format.add_argument('--clang-format-path', default='clang-format', help="path of clang-format used to check source")
-    parser_check_cppcheck = parser_check_subparsers.add_parser(
-        'cppcheck',
-        description="Check code with cppcheck (https://github.com/danmar/cppcheck)",
-        help="Check code with cppcheck (https://github.com/danmar/cppcheck)",
+    parser_check_tidy = parser_check_subparsers.add_parser(
+        'tidy',
+        description="Check code with clang-tidy",
+        help="Check code with clang-tidy",
         formatter_class=ArgumentDefaultsHelpFormatter,
     )
-    parser_check_cppcheck.set_defaults(func=cppcheck)
+    parser_check_tidy.set_defaults(func=clang_tidy)
+    parser_check_tidy.add_argument('dir', metavar='BUILD_DIR', nargs='?', default='build', help="directory to store cmake-generated and build files")
+    parser_check_tidy.add_argument('-j', '--jobs', default=4, metavar='N', help='execute N build jobs concurrently')
+    parser_check_tidy.add_argument('--clang-tidy-path', default='clang-tidy', help="path of clang-tidy used to check source")
+    parser_check_tidy.add_argument('--run-clang-tidy-path', default='run-clang-tidy', help="path of run-clang-tidy used to check source")
     parser_check_golangci_lint = parser_check_subparsers.add_parser(
         'golangci-lint',
         description="Check code with golangci-lint (https://golangci-lint.run/)",
@@ -322,6 +332,7 @@ def test_go(dir: str, rest: List[str]) -> None:
     parser_build.add_argument('--compiler', default='auto', choices=('auto', 'gcc', 'clang'), help="compiler used to build kvrocks")
     parser_build.add_argument('--cmake-path', default='cmake', help="path of cmake binary used to build kvrocks")
     parser_build.add_argument('-D', nargs='*', metavar='key=value', help='extra CMake definitions')
+    parser_build.add_argument('--skip-build', default=False, action='store_true', help='runs only the configure stage, skip the build stage')
     parser_build.set_defaults(func=build)
 
     parser_package = subparsers.add_parser(