Fix clang-tidy warnings (#1334)

.clang-tidy

@@ -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, modernize-loop-convert, cppcoreguidelines-init-variables, modernize-use-nullptr, cppcoreguidelines-macro-usage, cppcoreguidelines-narrowing-conversions
+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, cppcoreguidelines-init-variables, modernize-use-nullptr, cppcoreguidelines-macro-usage, cppcoreguidelines-narrowing-conversions, modernize-return-braced-init-list
 
 CheckOptions:
   - key:           cppcoreguidelines-special-member-functions.AllowSoleDefaultDtor

src/main.cc

@@ -275,13 +275,15 @@ bool isSupervisedMode(int mode) {
 static Status createPidFile(const std::string &path) {
   auto fd = UniqueFD(open(path.data(), O_RDWR | O_CREAT, 0660));
   if (!fd) {
-    return Status(Status::NotOK, strerror(errno));
+    return Status::FromErrno();
   }
+
   std::string pid_str = std::to_string(getpid());
   auto s = Util::Write(*fd, pid_str);
   if (!s.IsOK()) {
     return s.Prefixed("failed to write to PID-file");
   }
+
   return Status::OK();
 }
 

src/stats/disk_stats.cc

@@ -20,18 +20,12 @@
 
 #include "disk_stats.h"
 
-#include <algorithm>
 #include <memory>
 #include <string>
-#include <utility>
-#include <vector>
 
 #include "db_util.h"
 #include "rocksdb/status.h"
-#include "status.h"
 #include "storage/redis_metadata.h"
-#include "types/redis_bitmap.h"
-#include "types/redis_sortedint.h"
 #include "types/redis_zset.h"
 
 namespace Redis {

src/stats/stats.cc

@@ -53,7 +53,6 @@ int64_t Stats::GetMemoryRSS() {
 #else
 #include <fcntl.h>
 
-#include <cstdio>
 #include <cstring>
 #include <string>
 

src/storage/compact_filter.cc

@@ -28,6 +28,7 @@
 #include "types/redis_bitmap.h"
 
 namespace Engine {
+
 using rocksdb::Slice;
 
 bool MetadataFilter::Filter(int level, const Slice &key, const Slice &value, std::string *new_value,
@@ -66,21 +67,20 @@ Status SubKeyFilter::GetMetadata(const InternalKey &ikey, Metadata *metadata) co
       // metadata was deleted(perhaps compaction or manual)
       // clear the metadata
       cached_metadata_.clear();
-      return Status(Status::NotFound, "metadata is not found");
+      return {Status::NotFound, "metadata is not found"};
     } else {
       cached_key_.clear();
       cached_metadata_.clear();
-      return Status(Status::NotOK, "fetch error: " + s.ToString());
+      return {Status::NotOK, "fetch error: " + s.ToString()};
     }
   }
   // the metadata was not found
-  if (cached_metadata_.empty()) return Status(Status::NotFound, "metadata is not found");
+  if (cached_metadata_.empty()) return {Status::NotFound, "metadata is not found"};
   // the metadata is cached
   rocksdb::Status s = metadata->Decode(cached_metadata_);
   if (!s.ok()) {
     cached_key_.clear();
-    return Status(Status::NotOK, "decode error: " + s.ToString());
-    ;
+    return {Status::NotOK, "decode error: " + s.ToString()};
   }
   return Status::OK();
 }

src/storage/compact_filter.h

@@ -31,6 +31,7 @@
 #include "storage.h"
 
 namespace Engine {
+
 class MetadataFilter : public rocksdb::CompactionFilter {
  public:
   explicit MetadataFilter(Storage *storage) : stor_(storage) {}
@@ -122,4 +123,5 @@ class PubSubFilterFactory : public rocksdb::CompactionFilterFactory {
     return std::unique_ptr<rocksdb::CompactionFilter>(new PubSubFilter());
   }
 };
+
 }  // namespace Engine

src/storage/compaction_checker.cc

@@ -68,7 +68,7 @@ void CompactionChecker::PickCompactionFiles(const std::string &cf_name) {
     if (file_creation_time == 0) {
       // Fallback to the file Modification time to prevent repeatedly compacting the same file,
       // file_creation_time is 0 which means the unknown condition in rocksdb
-      auto s = rocksdb::Env::Default()->GetFileModificationTime(iter.first, &file_creation_time);
+      s = rocksdb::Env::Default()->GetFileModificationTime(iter.first, &file_creation_time);
       if (!s.ok()) {
         LOG(INFO) << "[compaction checker] Failed to get the file creation time: " << iter.first
                   << ", err: " << s.ToString();
@@ -107,7 +107,7 @@ void CompactionChecker::PickCompactionFiles(const std::string &cf_name) {
     // pick the file which was created more than 2 days
     if (file_creation_time < static_cast<uint64_t>(now - forceCompactSeconds)) {
       LOG(INFO) << "[compaction checker] Going to compact the key in file(created more than 2 days): " << iter.first;
-      auto s = storage_->Compact(&start_key, &stop_key);
+      s = storage_->Compact(&start_key, &stop_key);
       LOG(INFO) << "[compaction checker] Compact the key in file(created more than 2 days): " << iter.first
                 << " finished, result: " << s.ToString();
       maxFilesToCompact--;

src/storage/redis_db.cc

@@ -227,7 +227,7 @@ rocksdb::Status Database::Scan(const std::string &cursor, uint64_t limit, const
                                std::vector<std::string> *keys, std::string *end_cursor) {
   end_cursor->clear();
   uint64_t cnt = 0;
-  uint16_t slot_id = 0, slot_start = 0;
+  uint16_t slot_start = 0;
   std::string ns_prefix, ns_cursor, ns, user_key, value, index_key;
 
   LatestSnapShot ss(storage_);
@@ -259,7 +259,7 @@ rocksdb::Status Database::Scan(const std::string &cursor, uint64_t limit, const
     iter->Seek(ns_prefix);
   }
 
-  slot_id = slot_start;
+  uint16_t slot_id = slot_start;
   while (true) {
     for (; iter->Valid() && cnt < limit; iter->Next()) {
       if (!ns_prefix.empty() && !iter->key().starts_with(ns_prefix)) {
@@ -325,8 +325,8 @@ rocksdb::Status Database::RandomKey(const std::string &cursor, std::string *key)
     return s;
   }
   if (keys.empty() && !cursor.empty()) {
-    // if reach the end, restart from begining
-    auto s = Scan("", 60, "", &keys, &end_cursor);
+    // if reach the end, restart from beginning
+    s = Scan("", 60, "", &keys, &end_cursor);
     if (!s.ok()) {
       return s;
     }
@@ -414,13 +414,13 @@ rocksdb::Status Database::Dump(const Slice &user_key, std::vector<std::string> *
   infos->emplace_back(created_at_str + "." + std::to_string(created_at.tv_usec));
 
   if (metadata.Type() == kRedisList) {
-    ListMetadata metadata(false);
-    GetMetadata(kRedisList, ns_key, &metadata);
+    ListMetadata list_metadata(false);
+    GetMetadata(kRedisList, ns_key, &list_metadata);
     if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
     infos->emplace_back("head");
-    infos->emplace_back(std::to_string(metadata.head));
+    infos->emplace_back(std::to_string(list_metadata.head));
     infos->emplace_back("tail");
-    infos->emplace_back(std::to_string(metadata.tail));
+    infos->emplace_back(std::to_string(list_metadata.tail));
   }
 
   return rocksdb::Status::OK();
@@ -518,11 +518,11 @@ rocksdb::Status Database::ClearKeysOfSlot(const rocksdb::Slice &ns, int slot) {
 
 rocksdb::Status Database::GetSlotKeysInfo(int slot, std::map<int, uint64_t> *slotskeys, std::vector<std::string> *keys,
                                           int count) {
-  const rocksdb::Snapshot *snapshot = nullptr;
-  snapshot = storage_->GetDB()->GetSnapshot();
+  LatestSnapShot ss(storage_);
   rocksdb::ReadOptions read_options;
-  read_options.snapshot = snapshot;
+  read_options.snapshot = ss.GetSnapShot();
   storage_->SetReadOptions(read_options);
+
   auto iter = DBUtil::UniqueIterator(storage_, read_options, metadata_cf_handle_);
   bool end = false;
   for (int i = 0; i < HASH_SLOTS_SIZE; i++) {

src/storage/redis_metadata.cc

@@ -26,7 +26,6 @@
 #include <atomic>
 #include <cstdlib>
 #include <ctime>
-#include <vector>
 
 #include "cluster/redis_slot.h"
 #include "time_util.h"
@@ -184,9 +183,9 @@ void Metadata::InitVersionCounter() {
 }
 
 uint64_t Metadata::generateVersion() {
-  uint64_t version = Util::GetTimeStampUS();
+  uint64_t timestamp = Util::GetTimeStampUS();
   uint64_t counter = version_counter_.fetch_add(1);
-  return (version << VersionCounterBits) + (counter % (1 << VersionCounterBits));
+  return (timestamp << VersionCounterBits) + (counter % (1 << VersionCounterBits));
 }
 
 bool Metadata::operator==(const Metadata &that) const {

src/storage/redis_pubsub.cc

@@ -21,9 +21,11 @@
 #include "redis_pubsub.h"
 
 namespace Redis {
+
 rocksdb::Status PubSub::Publish(const Slice &channel, const Slice &value) {
   auto batch = storage_->GetWriteBatchBase();
   batch->Put(pubsub_cf_handle_, channel, value);
   return storage_->Write(storage_->DefaultWriteOptions(), batch->GetWriteBatch());
 }
+
 }  // namespace Redis

src/storage/scripting.cc

@@ -49,6 +49,7 @@ enum {
 };
 
 namespace Lua {
+
 lua_State *CreateState(bool read_only) {
   lua_State *lua = lua_open();
   loadLibraries(lua);

src/storage/table_properties_collector.cc

@@ -30,7 +30,6 @@
 rocksdb::Status CompactOnExpiredCollector::AddUserKey(const rocksdb::Slice &key, const rocksdb::Slice &value,
                                                       rocksdb::EntryType entry_type, rocksdb::SequenceNumber,
                                                       uint64_t) {
-  int now = 0;
   uint8_t type = 0;
   uint32_t expired = 0, subkeys = 0;
   uint64_t version = 0;
@@ -62,7 +61,7 @@ rocksdb::Status CompactOnExpiredCollector::AddUserKey(const rocksdb::Slice &key,
     GetFixed32(&cv, &subkeys);
   }
   total_keys_ += subkeys;
-  now = Server::GetCachedUnixTime();
+  int now = Server::GetCachedUnixTime();
   if ((expired > 0 && expired < static_cast<uint32_t>(now)) || (type != kRedisString && subkeys == 0)) {
     deleted_keys_ += subkeys + 1;
   }

src/types/geohash.cc

@@ -364,15 +364,14 @@ GeoHashRadius GeoHashHelper::GetAreasByRadius(double longitude, double latitude,
   GeoHashArea area;
   double min_lon = NAN, max_lon = NAN, min_lat = NAN, max_lat = NAN;
   double bounds[4];
-  int steps = 0;
 
   BoundingBox(longitude, latitude, radius_meters, bounds);
   min_lon = bounds[0];
   min_lat = bounds[1];
   max_lon = bounds[2];
   max_lat = bounds[3];
 
-  steps = EstimateStepsByRadius(radius_meters, latitude);
+  int steps = EstimateStepsByRadius(radius_meters, latitude);
 
   geohashGetCoordRange(&long_range, &lat_range);
   geohashEncode(&long_range, &lat_range, longitude, latitude, steps, &hash);

src/types/redis_bitmap.cc

@@ -122,7 +122,6 @@ rocksdb::Status Bitmap::GetString(const Slice &user_key, const uint32_t max_btos
   LatestSnapShot ss(storage_);
   read_options.snapshot = ss.GetSnapShot();
   storage_->SetReadOptions(read_options);
-  uint32_t frag_index = 0, valid_size = 0;
 
   auto iter = DBUtil::UniqueIterator(storage_, read_options);
   for (iter->Seek(prefix_key); iter->Valid() && iter->key().starts_with(prefix_key); iter->Next()) {
@@ -131,11 +130,11 @@ rocksdb::Status Bitmap::GetString(const Slice &user_key, const uint32_t max_btos
     if (!parse_result) {
       return rocksdb::Status::InvalidArgument(parse_result.Msg());
     }
-    frag_index = *parse_result;
+    uint32_t frag_index = *parse_result;
     fragment = iter->value().ToString();
     // To be compatible with data written before the commit d603b0e(#338)
     // and avoid returning extra null char after expansion.
-    valid_size = std::min(
+    uint32_t valid_size = std::min(
         {fragment.size(), static_cast<size_t>(kBitmapSegmentBytes), static_cast<size_t>(metadata.size - frag_index)});
 
     /*
@@ -166,7 +165,6 @@ rocksdb::Status Bitmap::GetString(const Slice &user_key, const uint32_t max_btos
     for (uint32_t i = 0; i < valid_size; i++) {
       if (!fragment[i]) continue;
       fragment[i] = static_cast<char>(swap_table[static_cast<uint8_t>(fragment[i])]);
-      ;
     }
     value->replace(frag_index, valid_size, fragment.data(), valid_size);
   }
@@ -352,14 +350,13 @@ rocksdb::Status Bitmap::BitOp(BitOpFlags op_flag, const std::string &op_name, co
   AppendNamespacePrefix(user_key, &ns_key);
   LockGuard guard(storage_->GetLockManager(), ns_key);
 
-  rocksdb::Status s;
   std::vector<std::pair<std::string, BitmapMetadata>> meta_pairs;
   uint64_t max_size = 0, num_keys = op_keys.size();
 
   for (const auto &op_key : op_keys) {
     BitmapMetadata metadata(false);
     AppendNamespacePrefix(op_key, &ns_op_key);
-    s = GetMetadata(ns_op_key, &metadata, &raw_value);
+    auto s = GetMetadata(ns_op_key, &metadata, &raw_value);
     if (!s.ok()) {
       if (s.IsNotFound()) {
         num_keys--;
@@ -388,7 +385,7 @@ rocksdb::Status Bitmap::BitOp(BitOpFlags op_flag, const std::string &op_name, co
 
   BitmapMetadata res_metadata;
   if (num_keys == op_keys.size() || op_flag != kBitOpAnd) {
-    uint64_t i = 0, frag_numkeys = num_keys, stop_index = (max_size - 1) / kBitmapSegmentBytes;
+    uint64_t frag_numkeys = num_keys, stop_index = (max_size - 1) / kBitmapSegmentBytes;
     std::unique_ptr<unsigned char[]> frag_res(new unsigned char[kBitmapSegmentBytes]);
     uint16_t frag_maxlen = 0, frag_minlen = 0;
     std::string sub_key, fragment;
@@ -432,14 +429,14 @@ rocksdb::Status Bitmap::BitOp(BitOpFlags op_flag, const std::string &op_name, co
         if (frag_minlen >= sizeof(uint64_t) * 4 && frag_numkeys <= 16) {
           auto *lres = reinterpret_cast<uint64_t *>(frag_res.get());
           const uint64_t *lp[16];
-          for (i = 0; i < frag_numkeys; i++) {
+          for (uint64_t i = 0; i < frag_numkeys; i++) {
             lp[i] = reinterpret_cast<const uint64_t *>(fragments[i].data());
           }
           memcpy(frag_res.get(), fragments[0].data(), frag_minlen);
 
           if (op_flag == kBitOpAnd) {
             while (frag_minlen >= sizeof(uint64_t) * 4) {
-              for (i = 1; i < frag_numkeys; i++) {
+              for (uint64_t i = 1; i < frag_numkeys; i++) {
                 lres[0] &= lp[i][0];
                 lres[1] &= lp[i][1];
                 lres[2] &= lp[i][2];
@@ -452,7 +449,7 @@ rocksdb::Status Bitmap::BitOp(BitOpFlags op_flag, const std::string &op_name, co
             }
           } else if (op_flag == kBitOpOr) {
             while (frag_minlen >= sizeof(uint64_t) * 4) {
-              for (i = 1; i < frag_numkeys; i++) {
+              for (uint64_t i = 1; i < frag_numkeys; i++) {
                 lres[0] |= lp[i][0];
                 lres[1] |= lp[i][1];
                 lres[2] |= lp[i][2];
@@ -465,7 +462,7 @@ rocksdb::Status Bitmap::BitOp(BitOpFlags op_flag, const std::string &op_name, co
             }
           } else if (op_flag == kBitOpXor) {
             while (frag_minlen >= sizeof(uint64_t) * 4) {
-              for (i = 1; i < frag_numkeys; i++) {
+              for (uint64_t i = 1; i < frag_numkeys; i++) {
                 lres[0] ^= lp[i][0];
                 lres[1] ^= lp[i][1];
                 lres[2] ^= lp[i][2];
@@ -493,7 +490,7 @@ rocksdb::Status Bitmap::BitOp(BitOpFlags op_flag, const std::string &op_name, co
         for (; j < frag_maxlen; j++) {
           output = (fragments[0].size() <= j) ? 0 : fragments[0][j];
           if (op_flag == kBitOpNot) output = ~output;
-          for (i = 1; i < frag_numkeys; i++) {
+          for (uint64_t i = 1; i < frag_numkeys; i++) {
             byte = (fragments[i].size() <= j) ? 0 : fragments[i][j];
             switch (op_flag) {
               case kBitOpAnd: