From 4e7a182d09fed9d38ea666c78817e59aeeb6fbe6 Mon Sep 17 00:00:00 2001 From: jsteemann Date: Thu, 15 Feb 2018 16:43:23 -0800 Subject: [PATCH] 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 | 6 ++---- db/memtable.cc | 6 +++--- db/table_cache.cc | 3 +-- db/table_properties_collector.cc | 2 +- env/env_posix.cc | 2 +- table/block.cc | 3 +-- table/block_based_filter_block.cc | 2 +- table/block_based_table_builder.cc | 1 - table/format.cc | 2 +- table/plain_table_factory.cc | 2 +- util/string_util.cc | 4 ++-- utilities/transactions/transaction_lock_mgr.cc | 3 +-- 12 files changed, 15 insertions(+), 21 deletions(-) diff --git a/db/column_family.cc b/db/column_family.cc index ad5019e80..ea5eaa468 100644 --- a/db/column_family.cc +++ b/db/column_family.cc @@ -898,8 +898,7 @@ Compaction* ColumnFamilyData::CompactRange( SuperVersion* ColumnFamilyData::GetReferencedSuperVersion( InstrumentedMutex* db_mutex) { - SuperVersion* sv = nullptr; - sv = GetThreadLocalSuperVersion(db_mutex); + SuperVersion* 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(ptr); + SuperVersion* sv = static_cast(ptr); if (sv == SuperVersion::kSVObsolete || sv->version_number != super_version_number_.load()) { RecordTick(ioptions_.statistics, NUMBER_SUPERVERSION_ACQUIRES); diff --git a/db/memtable.cc b/db/memtable.cc index 4aaffde25..80671a41b 100644 --- a/db/memtable.cc +++ b/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_ || - (pinned_iters_mgr_ && !pinned_iters_mgr_->PinningEnabled())); + assert(!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(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 diff --git a/db/table_cache.cc b/db/table_cache.cc index 07daec2d6..56b8272d4 100644 --- a/db/table_cache.cc +++ b/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 */, diff --git a/db/table_properties_collector.cc b/db/table_properties_collector.cc index 6247852dc..fc27844b8 100644 --- a/db/table_properties_collector.cc +++ b/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()) { diff --git a/env/env_posix.cc b/env/env_posix.cc index b30a00e4b..c0e936033 100644 --- a/env/env_posix.cc +++ b/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(); } diff --git a/table/block.cc b/table/block.cc index 939eabba7..ae82ae337 100644 --- a/table/block.cc +++ b/table/block.cc @@ -167,8 +167,7 @@ void BlockIter::SeekForPrev(const Slice& target) { return; } uint32_t index = 0; - bool ok = false; - ok = BinarySeek(target, 0, num_restarts_ - 1, &index); + bool ok = BinarySeek(target, 0, num_restarts_ - 1, &index); if (!ok) { return; diff --git a/table/block_based_filter_block.cc b/table/block_based_filter_block.cc index 697c11a42..6e300e810 100644 --- a/table/block_based_filter_block.cc +++ b/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"); diff --git a/table/block_based_table_builder.cc b/table/block_based_table_builder.cc index 7b030382f..71f01b9b0 100644 --- a/table/block_based_table_builder.cc +++ b/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) { diff --git a/table/format.cc b/table/format.cc index 6ab2fd5af..534d8c466 100644 --- a/table/format.cc +++ b/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_); diff --git a/table/plain_table_factory.cc b/table/plain_table_factory.cc index e50e5413d..7cf71b0e5 100644 --- a/table/plain_table_factory.cc +++ b/table/plain_table_factory.cc @@ -102,7 +102,7 @@ Status GetMemTableRepFactoryFromString( std::vector 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); } diff --git a/util/string_util.cc b/util/string_util.cc index a37605aa0..f3581105e 100644 --- a/util/string_util.cc +++ b/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) { diff --git a/utilities/transactions/transaction_lock_mgr.cc b/utilities/transactions/transaction_lock_mgr.cc index 9485067d5..4d46d6e8e 100644 --- a/utilities/transactions/transaction_lock_mgr.cc +++ b/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; }