Some minor fix-ups (#6440)

Summary:
Cleanup some code without any real change in functionality.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6440

Differential Revision: D20015891

Pulled By: riversand963

fbshipit-source-id: 33e18754b0f002006a6d4805e9aaf84c0c8ad25a

db/arena_wrapped_db_iter.h

@@ -45,26 +45,26 @@ class ArenaWrappedDBIter : public Iterator {
   // Set the internal iterator wrapped inside the DB Iterator. Usually it is
   // a merging iterator.
   virtual void SetIterUnderDBIter(InternalIterator* iter) {
-    static_cast<DBIter*>(db_iter_)->SetIter(iter);
+    db_iter_->SetIter(iter);
   }
 
-  virtual bool Valid() const override { return db_iter_->Valid(); }
+  bool Valid() const override { return db_iter_->Valid(); }
-  virtual void SeekToFirst() override { db_iter_->SeekToFirst(); }
+  void SeekToFirst() override { db_iter_->SeekToFirst(); }
-  virtual void SeekToLast() override { db_iter_->SeekToLast(); }
+  void SeekToLast() override { db_iter_->SeekToLast(); }
-  virtual void Seek(const Slice& target) override { db_iter_->Seek(target); }
+  void Seek(const Slice& target) override { db_iter_->Seek(target); }
-  virtual void SeekForPrev(const Slice& target) override {
+  void SeekForPrev(const Slice& target) override {
     db_iter_->SeekForPrev(target);
   }
-  virtual void Next() override { db_iter_->Next(); }
+  void Next() override { db_iter_->Next(); }
-  virtual void Prev() override { db_iter_->Prev(); }
+  void Prev() override { db_iter_->Prev(); }
-  virtual Slice key() const override { return db_iter_->key(); }
+  Slice key() const override { return db_iter_->key(); }
-  virtual Slice value() const override { return db_iter_->value(); }
+  Slice value() const override { return db_iter_->value(); }
-  virtual Status status() const override { return db_iter_->status(); }
+  Status status() const override { return db_iter_->status(); }
   bool IsBlob() const { return db_iter_->IsBlob(); }
 
-  virtual Status GetProperty(std::string prop_name, std::string* prop) override;
+  Status GetProperty(std::string prop_name, std::string* prop) override;
 
-  virtual Status Refresh() override;
+  Status Refresh() override;
 
   void Init(Env* env, const ReadOptions& read_options,
             const ImmutableCFOptions& cf_options,

db/db_impl/db_impl.cc

@@ -2561,7 +2561,7 @@ ArenaWrappedDBIter* DBImpl::NewIteratorImpl(const ReadOptions& read_options,
       env_, read_options, *cfd->ioptions(), sv->mutable_cf_options, snapshot,
       sv->mutable_cf_options.max_sequential_skip_in_iterations,
       sv->version_number, read_callback, this, cfd, allow_blob,
-      ((read_options.snapshot != nullptr) ? false : allow_refresh));
+      read_options.snapshot != nullptr ? false : allow_refresh);
 
   InternalIterator* internal_iter =
       NewInternalIterator(read_options, cfd, sv, db_iter->GetArena(),

db/db_iter.cc

@@ -33,19 +33,6 @@
 
 namespace ROCKSDB_NAMESPACE {
 
-#if 0
-static void DumpInternalIter(Iterator* iter) {
-  for (iter->SeekToFirst(); iter->Valid(); iter->Next()) {
-    ParsedInternalKey k;
-    if (!ParseInternalKey(iter->key(), &k)) {
-      fprintf(stderr, "Corrupt '%s'\n", EscapeString(iter->key()).c_str());
-    } else {
-      fprintf(stderr, "@ '%s'\n", k.DebugString().c_str());
-    }
-  }
-}
-#endif
-
 DBIter::DBIter(Env* _env, const ReadOptions& read_options,
                const ImmutableCFOptions& cf_options,
                const MutableCFOptions& mutable_cf_options,
@@ -183,7 +170,7 @@ void DBIter::Next() {
 //       a delete marker or a sequence number higher than sequence_
 //       saved_key_ MUST have a proper user_key before calling this function
 //
-// The prefix parameter, if not null, indicates that we need to iterator
+// The prefix parameter, if not null, indicates that we need to iterate
 // within the prefix, and the iterator needs to be made invalid, if no
 // more entry for the prefix can be found.
 bool DBIter::FindNextUserEntry(bool skipping_saved_key, const Slice* prefix) {
@@ -204,13 +191,14 @@ bool DBIter::FindNextUserEntryInternal(bool skipping_saved_key,
   // or equal to saved_key_. We could skip these entries either because
   // sequence numbers were too high or because skipping_saved_key = true.
   // What saved_key_ contains throughout this method:
-  //  - if skipping_saved_key        : saved_key_ contains the key that we need
+  //  - if skipping_saved_key : saved_key_ contains the key that we need
-  //  to skip,
+  //                            to skip, and we haven't seen any keys greater
-  //                         and we haven't seen any keys greater than that,
+  //                            than that,
-  //  - if num_skipped > 0 : saved_key_ contains the key that we have skipped
+  //  - if num_skipped > 0    : saved_key_ contains the key that we have skipped
-  //                         num_skipped times, and we haven't seen any keys
+  //                            num_skipped times, and we haven't seen any keys
-  //                         greater than that,
+  //                            greater than that,
-  //  - none of the above  : saved_key_ can contain anything, it doesn't matter.
+  //  - none of the above     : saved_key_ can contain anything, it doesn't
+  //                            matter.
   uint64_t num_skipped = 0;
   // For write unprepared, the target sequence number in reseek could be larger
   // than the snapshot, and thus needs to be skipped again. This could result in
@@ -292,9 +280,7 @@ bool DBIter::FindNextUserEntryInternal(bool skipping_saved_key,
             if (start_seqnum_ > 0) {
               // we are taking incremental snapshot here
               // incremental snapshots aren't supported on DB with range deletes
-              assert(!(
+              assert(ikey_.type != kTypeBlobIndex);
-                (ikey_.type == kTypeBlobIndex) && (start_seqnum_ > 0)
-              ));
               if (ikey_.sequence >= start_seqnum_) {
                 saved_key_.SetInternalKey(ikey_);
                 valid_ = true;
@@ -372,7 +358,7 @@ bool DBIter::FindNextUserEntryInternal(bool skipping_saved_key,
       // to seek to the target sequence number.
       int cmp =
           user_comparator_.Compare(ikey_.user_key, saved_key_.GetUserKey());
-      if (cmp == 0 || (skipping_saved_key && cmp <= 0)) {
+      if (cmp == 0 || (skipping_saved_key && cmp < 0)) {
         num_skipped++;
       } else {
         saved_key_.SetUserKey(
@@ -391,7 +377,7 @@ bool DBIter::FindNextUserEntryInternal(bool skipping_saved_key,
     // reseek previously.
     //
     // TODO(lth): If we reseek to sequence number greater than ikey_.sequence,
-    // than it does not make sense to reseek as we would actually land further
+    // then it does not make sense to reseek as we would actually land further
     // away from the desired key. There is opportunity for optimization here.
     if (num_skipped > max_skip_ && !reseek_done) {
       is_key_seqnum_zero_ = false;
@@ -1130,18 +1116,16 @@ void DBIter::Seek(const Slice& target) {
 
   // Now the inner iterator is placed to the target position. From there,
   // we need to find out the next key that is visible to the user.
-  //
   ClearSavedValue();
   if (prefix_same_as_start_) {
     // The case where the iterator needs to be invalidated if it has exausted
     // keys within the same prefix of the seek key.
     assert(prefix_extractor_ != nullptr);
-    Slice target_prefix;
+    Slice target_prefix = prefix_extractor_->Transform(target);
-    target_prefix = prefix_extractor_->Transform(target);
     FindNextUserEntry(false /* not skipping saved_key */,
                       &target_prefix /* prefix */);
     if (valid_) {
-      // Remember the prefix of the seek key for the future Prev() call to
+      // Remember the prefix of the seek key for the future Next() call to
       // check.
       prefix_.SetUserKey(target_prefix);
     }
@@ -1197,8 +1181,7 @@ void DBIter::SeekForPrev(const Slice& target) {
     // The case where the iterator needs to be invalidated if it has exausted
     // keys within the same prefix of the seek key.
     assert(prefix_extractor_ != nullptr);
-    Slice target_prefix;
+    Slice target_prefix = prefix_extractor_->Transform(target);
-    target_prefix = prefix_extractor_->Transform(target);
     PrevInternal(&target_prefix);
     if (valid_) {
       // Remember the prefix of the seek key for the future Prev() call to

db/db_iter.h

@@ -25,10 +25,10 @@ namespace ROCKSDB_NAMESPACE {
 // This file declares the factory functions of DBIter, in its original form
 // or a wrapped form with class ArenaWrappedDBIter, which is defined here.
 // Class DBIter, which is declared and implemented inside db_iter.cc, is
-// a iterator that converts internal keys (yielded by an InternalIterator)
+// an iterator that converts internal keys (yielded by an InternalIterator)
 // that were live at the specified sequence number into appropriate user
 // keys.
-// Each internal key is consist of a user key, a sequence number, and a value
+// Each internal key consists of a user key, a sequence number, and a value
 // type. DBIter deals with multiple key versions, tombstones, merge operands,
 // etc, and exposes an Iterator.
 // For example, DBIter may wrap following InternalIterator:
@@ -133,14 +133,12 @@ class DBIter final : public Iterator {
     local_stats_.BumpGlobalStatistics(statistics_);
     iter_.DeleteIter(arena_mode_);
   }
-  virtual void SetIter(InternalIterator* iter) {
+  void SetIter(InternalIterator* iter) {
     assert(iter_.iter() == nullptr);
     iter_.Set(iter);
     iter_.iter()->SetPinnedItersMgr(&pinned_iters_mgr_);
   }
-  virtual ReadRangeDelAggregator* GetRangeDelAggregator() {
+  ReadRangeDelAggregator* GetRangeDelAggregator() { return &range_del_agg_; }
-    return &range_del_agg_;
-  }
 
   bool Valid() const override { return valid_; }
   Slice key() const override {
@@ -184,7 +182,7 @@ class DBIter final : public Iterator {
   void SeekForPrev(const Slice& target) final override;
   void SeekToFirst() final override;
   void SeekToLast() final override;
-  Env* env() { return env_; }
+  Env* env() const { return env_; }
   void set_sequence(uint64_t s) {
     sequence_ = s;
     if (read_callback_) {
@@ -202,10 +200,10 @@ class DBIter final : public Iterator {
   bool ReverseToBackward();
   // Set saved_key_ to the seek key to target, with proper sequence number set.
   // It might get adjusted if the seek key is smaller than iterator lower bound.
-  void SetSavedKeyToSeekTarget(const Slice& /*target*/);
+  void SetSavedKeyToSeekTarget(const Slice& target);
   // Set saved_key_ to the seek key to target, with proper sequence number set.
   // It might get adjusted if the seek key is larger than iterator upper bound.
-  void SetSavedKeyToSeekForPrevTarget(const Slice& /*target*/);
+  void SetSavedKeyToSeekForPrevTarget(const Slice& target);
   bool FindValueForCurrentKey();
   bool FindValueForCurrentKeyUsingSeek();
   bool FindUserKeyBeforeSavedKey();
@@ -221,7 +219,7 @@ class DBIter final : public Iterator {
 
   // If prefix is not null, we need to set the iterator to invalid if no more
   // entry can be found within the prefix.
-  void PrevInternal(const Slice* /*prefix*/);
+  void PrevInternal(const Slice* prefix);
   bool TooManyInternalKeysSkipped(bool increment = true);
   bool IsVisible(SequenceNumber sequence);
 
@@ -330,6 +328,7 @@ class DBIter final : public Iterator {
   // if this value > 0 iterator will return internal keys
   SequenceNumber start_seqnum_;
 };
+
 // Return a new iterator that converts internal keys (yielded by
 // "*internal_iter") that were live at the specified `sequence` number
 // into appropriate user keys.

table/block_based/block.cc

@@ -156,12 +156,9 @@ void IndexBlockIter::Prev() {
     restart_index_--;
   }
   SeekToRestartPoint(restart_index_);
-  do {
+  // Loop until end of current entry hits the start of original entry
-    if (!ParseNextIndexKey()) {
+  while (ParseNextIndexKey() && NextEntryOffset() < original) {
-      break;
+  }
-    }
-    // Loop until end of current entry hits the start of original entry
-  } while (NextEntryOffset() < original);
 }
 
 // Similar to IndexBlockIter::Prev but also caches the prev entries
@@ -253,12 +250,9 @@ void DataBlockIter::Seek(const Slice& target) {
     return;
   }
   SeekToRestartPoint(index);
-  // Linear search (within restart block) for first key >= target
 
-  while (true) {
+  // Linear search (within restart block) for first key >= target
-    if (!ParseNextDataKey<DecodeEntry>() || Compare(key_, seek_key) >= 0) {
+  while (ParseNextDataKey<DecodeEntry>() && Compare(key_, seek_key) < 0) {
-      return;
-    }
   }
 }
 
@@ -415,12 +409,9 @@ void IndexBlockIter::Seek(const Slice& target) {
     return;
   }
   SeekToRestartPoint(index);
-  // Linear search (within restart block) for first key >= target
 
-  while (true) {
+  // Linear search (within restart block) for first key >= target
-    if (!ParseNextIndexKey() || Compare(key_, seek_key) >= 0) {
+  while (ParseNextIndexKey() && Compare(key_, seek_key) < 0) {
-      return;
-    }
   }
 }
 
@@ -438,8 +429,8 @@ void DataBlockIter::SeekForPrev(const Slice& target) {
     return;
   }
   SeekToRestartPoint(index);
-  // Linear search (within restart block) for first key >= seek_key
 
+  // Linear search (within restart block) for first key >= seek_key
   while (ParseNextDataKey<DecodeEntry>() && Compare(key_, seek_key) < 0) {
   }
   if (!Valid()) {

table/block_based/block.h

@@ -188,7 +188,6 @@ class Block {
   // NOTE: for the hash based lookup, if a key prefix doesn't match any key,
   // the iterator will simply be set as "invalid", rather than returning
   // the key that is just pass the target key.
-
   DataBlockIter* NewDataIterator(const Comparator* comparator,
                                  const Comparator* user_comparator,
                                  DataBlockIter* iter = nullptr,
@@ -269,31 +268,30 @@ class BlockIter : public InternalIteratorBase<TValue> {
     Cleanable::Reset();
   }
 
-  virtual bool Valid() const override { return current_ < restarts_; }
+  bool Valid() const override { return current_ < restarts_; }
-  virtual Status status() const override { return status_; }
+  Status status() const override { return status_; }
-  virtual Slice key() const override {
+  Slice key() const override {
     assert(Valid());
     return key_.GetKey();
   }
 
 #ifndef NDEBUG
-  virtual ~BlockIter() {
+  ~BlockIter() override {
     // Assert that the BlockIter is never deleted while Pinning is Enabled.
     assert(!pinned_iters_mgr_ ||
            (pinned_iters_mgr_ && !pinned_iters_mgr_->PinningEnabled()));
   }
-  virtual void SetPinnedItersMgr(
+  void SetPinnedItersMgr(PinnedIteratorsManager* pinned_iters_mgr) override {
-      PinnedIteratorsManager* pinned_iters_mgr) override {
     pinned_iters_mgr_ = pinned_iters_mgr;
   }
   PinnedIteratorsManager* pinned_iters_mgr_ = nullptr;
 #endif
 
-  virtual bool IsKeyPinned() const override {
+  bool IsKeyPinned() const override {
     return block_contents_pinned_ && key_pinned_;
   }
 
-  virtual bool IsValuePinned() const override { return block_contents_pinned_; }
+  bool IsValuePinned() const override { return block_contents_pinned_; }
 
   size_t TEST_CurrentEntrySize() { return NextEntryOffset() - current_; }
 
@@ -394,7 +392,7 @@ class DataBlockIter final : public BlockIter<Slice> {
     data_block_hash_index_ = data_block_hash_index;
   }
 
-  virtual Slice value() const override {
+  Slice value() const override {
     assert(Valid());
     if (read_amp_bitmap_ && current_ < restarts_ &&
         current_ != last_bitmap_offset_) {
@@ -405,7 +403,7 @@ class DataBlockIter final : public BlockIter<Slice> {
     return value_;
   }
 
-  virtual void Seek(const Slice& target) override;
+  void Seek(const Slice& target) override;
 
   inline bool SeekForGet(const Slice& target) {
     if (!data_block_hash_index_) {
@@ -416,25 +414,25 @@ class DataBlockIter final : public BlockIter<Slice> {
     return SeekForGetImpl(target);
   }
 
-  virtual void SeekForPrev(const Slice& target) override;
+  void SeekForPrev(const Slice& target) override;
 
-  virtual void Prev() override;
+  void Prev() override;
 
-  virtual void Next() final override;
+  void Next() final override;
 
   // Try to advance to the next entry in the block. If there is data corruption
   // or error, report it to the caller instead of aborting the process. May
   // incur higher CPU overhead because we need to perform check on every entry.
   void NextOrReport();
 
-  virtual void SeekToFirst() override;
+  void SeekToFirst() override;
 
   // Try to seek to the first entry in the block. If there is data corruption
   // or error, report it to caller instead of aborting the process. May incur
   // higher CPU overhead because we need to perform check on every entry.
   void SeekToFirstOrReport();
 
-  virtual void SeekToLast() override;
+  void SeekToLast() override;
 
   void Invalidate(Status s) {
     InvalidateBase(s);
@@ -490,7 +488,7 @@ class IndexBlockIter final : public BlockIter<IndexValue> {
  public:
   IndexBlockIter() : BlockIter(), prefix_index_(nullptr) {}
 
-  virtual Slice key() const override {
+  Slice key() const override {
     assert(Valid());
     return key_.GetKey();
   }
@@ -526,7 +524,7 @@ class IndexBlockIter final : public BlockIter<IndexValue> {
     return key();
   }
 
-  virtual IndexValue value() const override {
+  IndexValue value() const override {
     assert(Valid());
     if (value_delta_encoded_ || global_seqno_state_ != nullptr) {
       return decoded_value_;
@@ -547,9 +545,9 @@ class IndexBlockIter final : public BlockIter<IndexValue> {
   // If the prefix of `target` doesn't exist in the file, it can either
   // return the result of total order seek, or set both of Valid() = false
   // and status() = NotFound().
-  virtual void Seek(const Slice& target) override;
+  void Seek(const Slice& target) override;
 
-  virtual void SeekForPrev(const Slice&) override {
+  void SeekForPrev(const Slice&) override {
     assert(false);
     current_ = restarts_;
     restart_index_ = num_restarts_;
@@ -560,13 +558,13 @@ class IndexBlockIter final : public BlockIter<IndexValue> {
     value_.clear();
   }
 
-  virtual void Prev() override;
+  void Prev() override;
 
-  virtual void Next() override;
+  void Next() override;
 
-  virtual void SeekToFirst() override;
+  void SeekToFirst() override;
 
-  virtual void SeekToLast() override;
+  void SeekToLast() override;
 
   void Invalidate(Status s) { InvalidateBase(s); }
 

table/block_based/block_based_table_reader.cc

@@ -54,6 +54,7 @@
 #include "util/crc32c.h"
 #include "util/stop_watch.h"
 #include "util/string_util.h"
+#include "util/util.h"
 #include "util/xxhash.h"
 
 namespace ROCKSDB_NAMESPACE {
@@ -4047,6 +4048,7 @@ Status BlockBasedTable::CreateIndexReader(
                                           index_reader);
     }
     case BlockBasedTableOptions::kBinarySearch:
+      FALLTHROUGH_INTENDED;
     case BlockBasedTableOptions::kBinarySearchWithFirstKey: {
       return BinarySearchIndexReader::Create(this, prefetch_buffer, use_cache,
                                              prefetch, pin, lookup_context,

table/iterator_wrapper.h

@@ -56,7 +56,7 @@ class IteratorWrapperBase {
   }
 
   // Iterator interface methods
-  bool Valid() const        { return valid_; }
+  bool Valid() const { return valid_; }
   Slice key() const {
     assert(Valid());
     return result_.key;
@@ -66,13 +66,20 @@ class IteratorWrapperBase {
     return iter_->value();
   }
   // Methods below require iter() != nullptr
-  Status status() const     { assert(iter_); return iter_->status(); }
+  Status status() const {
+    assert(iter_);
+    return iter_->status();
+  }
   void Next() {
     assert(iter_);
     valid_ = iter_->NextAndGetResult(&result_);
     assert(!valid_ || iter_->status().ok());
   }
-  void Prev()               { assert(iter_); iter_->Prev();        Update(); }
+  void Prev() {
+    assert(iter_);
+    iter_->Prev();
+    Update();
+  }
   void Seek(const Slice& k) {
     assert(iter_);
     iter_->Seek(k);
@@ -83,8 +90,16 @@ class IteratorWrapperBase {
     iter_->SeekForPrev(k);
     Update();
   }
-  void SeekToFirst()        { assert(iter_); iter_->SeekToFirst(); Update(); }
+  void SeekToFirst() {
-  void SeekToLast()         { assert(iter_); iter_->SeekToLast();  Update(); }
+    assert(iter_);
+    iter_->SeekToFirst();
+    Update();
+  }
+  void SeekToLast() {
+    assert(iter_);
+    iter_->SeekToLast();
+    Update();
+  }
 
   bool MayBeOutOfLowerBound() {
     assert(Valid());

table/merging_iterator.cc

@@ -167,7 +167,6 @@ class MergingIterator : public InternalIterator {
       SwitchToForward();
       // The loop advanced all non-current children to be > key() so current_
       // should still be strictly the smallest key.
-      assert(current_ == CurrentForward());
     }
 
     // For the heap modifications below to be correct, current_ must be the