Several small "fixes"

Summary:
- removed a few unneeded variables
- fused some variable declarations and their assignments
- fixed right-trimming code in string_util.cc to not underflow
- simplifed an assertion
- move non-nullptr check assertion before dereferencing of that pointer
- pass an std::string function parameter by const reference instead of by value (avoiding potential copy)
Closes https://github.com/facebook/rocksdb/pull/3507

Differential Revision: D7004679

Pulled By: sagar0

fbshipit-source-id: 52944952d9b56dfcac3bea3cd7878e315bb563c4

db/column_family.cc

@@ -898,8 +898,7 @@ Compaction* ColumnFamilyData::CompactRange(
 
 SuperVersion* ColumnFamilyData::GetReferencedSuperVersion(
     InstrumentedMutex* db_mutex) {
-  SuperVersion* sv = nullptr;
+  SuperVersion* sv = GetThreadLocalSuperVersion(db_mutex);
-  sv = GetThreadLocalSuperVersion(db_mutex);
   sv->Ref();
   if (!ReturnThreadLocalSuperVersion(sv)) {
     // This Unref() corresponds to the Ref() in GetThreadLocalSuperVersion()
@@ -913,7 +912,6 @@ SuperVersion* ColumnFamilyData::GetReferencedSuperVersion(
 
 SuperVersion* ColumnFamilyData::GetThreadLocalSuperVersion(
     InstrumentedMutex* db_mutex) {
-  SuperVersion* sv = nullptr;
   // The SuperVersion is cached in thread local storage to avoid acquiring
   // mutex when SuperVersion does not change since the last use. When a new
   // SuperVersion is installed, the compaction or flush thread cleans up
@@ -932,7 +930,7 @@ SuperVersion* ColumnFamilyData::GetThreadLocalSuperVersion(
   // should only keep kSVInUse before ReturnThreadLocalSuperVersion call
   // (if no Scrape happens).
   assert(ptr != SuperVersion::kSVInUse);
-  sv = static_cast<SuperVersion*>(ptr);
+  SuperVersion* sv = static_cast<SuperVersion*>(ptr);
   if (sv == SuperVersion::kSVObsolete ||
       sv->version_number != super_version_number_.load()) {
     RecordTick(ioptions_.statistics, NUMBER_SUPERVERSION_ACQUIRES);

db/memtable.cc

@@ -289,8 +289,7 @@ class MemTableIterator : public InternalIterator {
 #ifndef NDEBUG
     // Assert that the MemTableIterator is never deleted while
     // Pinning is Enabled.
-    assert(!pinned_iters_mgr_ ||
+    assert(!pinned_iters_mgr_ || !pinned_iters_mgr_->PinningEnabled());
-           (pinned_iters_mgr_ && !pinned_iters_mgr_->PinningEnabled()));
 #endif
     if (arena_mode_) {
       iter_->~Iterator();
@@ -589,11 +588,12 @@ struct Saver {
 
 static bool SaveValue(void* arg, const char* entry) {
   Saver* s = reinterpret_cast<Saver*>(arg);
+  assert(s != nullptr);
   MergeContext* merge_context = s->merge_context;
   RangeDelAggregator* range_del_agg = s->range_del_agg;
   const MergeOperator* merge_operator = s->merge_operator;
 
-  assert(s != nullptr && merge_context != nullptr && range_del_agg != nullptr);
+  assert(merge_context != nullptr && range_del_agg != nullptr);
 
   // entry format is:
   //    klength  varint32

db/table_cache.cc

@@ -272,9 +272,8 @@ InternalIterator* TableCache::NewRangeTombstoneIterator(
     const InternalKeyComparator& icomparator, const FileDescriptor& fd,
     HistogramImpl* file_read_hist, bool skip_filters, int level) {
   Status s;
-  TableReader* table_reader = nullptr;
   Cache::Handle* handle = nullptr;
-  table_reader = fd.table_reader;
+  TableReader* table_reader = fd.table_reader;
   if (table_reader == nullptr) {
     s = FindTable(env_options, icomparator, fd, &handle,
                   options.read_tier == kBlockCacheTier /* no_io */,

db/table_properties_collector.cc

@@ -60,7 +60,7 @@ InternalKeyPropertiesCollector::GetReadableProperties() const {
 namespace {
 
 uint64_t GetUint64Property(const UserCollectedProperties& props,
-                           const std::string property_name,
+                           const std::string& property_name,
                            bool* property_present) {
   auto pos = props.find(property_name);
   if (pos == props.end()) {

env/env_posix.cc

@@ -763,7 +763,7 @@ class PosixEnv : public Env {
 
   virtual Status GetAbsolutePath(const std::string& db_path,
                                  std::string* output_path) override {
-    if (db_path.find('/') == 0) {
+    if (!db_path.empty() && db_path[0] == '/') {
       *output_path = db_path;
       return Status::OK();
     }

table/block.cc

@@ -167,8 +167,7 @@ void BlockIter::SeekForPrev(const Slice& target) {
     return;
   }
   uint32_t index = 0;
-  bool ok = false;
+  bool ok = BinarySeek(target, 0, num_restarts_ - 1, &index);
-  ok = BinarySeek(target, 0, num_restarts_ - 1, &index);
 
   if (!ok) {
     return;

table/block_based_filter_block.cc

@@ -233,7 +233,7 @@ size_t BlockBasedFilterBlockReader::ApproximateMemoryUsage() const {
 }
 
 std::string BlockBasedFilterBlockReader::ToString() const {
-  std::string result, filter_meta;
+  std::string result;
   result.reserve(1024);
 
   std::string s_bo("Block offset"), s_hd("Hex dump"), s_fb("# filter blocks");

table/block_based_table_builder.cc

@@ -724,7 +724,6 @@ Status BlockBasedTableBuilder::Finish() {
               : "nullptr";
 
       std::string property_collectors_names = "[";
-      property_collectors_names = "[";
       for (size_t i = 0;
            i < r->ioptions.table_properties_collector_factories.size(); ++i) {
         if (i != 0) {

table/format.cc

@@ -196,7 +196,7 @@ Status Footer::DecodeFrom(Slice* input) {
 }
 
 std::string Footer::ToString() const {
-  std::string result, handle_;
+  std::string result;
   result.reserve(1024);
 
   bool legacy = IsLegacyFooterFormat(table_magic_number_);

table/plain_table_factory.cc

@@ -102,7 +102,7 @@ Status GetMemTableRepFactoryFromString(
   std::vector<std::string> opts_list = StringSplit(opts_str, ':');
   size_t len = opts_list.size();
 
-  if (opts_list.size() <= 0 || opts_list.size() > 2) {
+  if (opts_list.empty() || opts_list.size() > 2) {
     return Status::InvalidArgument("Can't parse memtable_factory option ",
                                    opts_str);
   }

util/string_util.cc

@@ -244,10 +244,10 @@ std::string trim(const std::string& str) {
   if (str.empty()) return std::string();
   size_t start = 0;
   size_t end = str.size() - 1;
-  while (isspace(str[start]) != 0 && start <= end) {
+  while (isspace(str[start]) != 0 && start < end) {
     ++start;
   }
-  while (isspace(str[end]) != 0 && start <= end) {
+  while (isspace(str[end]) != 0 && start < end) {
     --end;
   }
   if (start <= end) {

utilities/transactions/transaction_lock_mgr.cc

@@ -321,11 +321,10 @@ Status TransactionLockMgr::AcquireWithTimeout(
     uint32_t column_family_id, const std::string& key, Env* env,
     int64_t timeout, const LockInfo& lock_info) {
   Status result;
-  uint64_t start_time = 0;
   uint64_t end_time = 0;
 
   if (timeout > 0) {
-    start_time = env->NowMicros();
+    uint64_t start_time = env->NowMicros();
     end_time = start_time + timeout;
   }