Remove some unnecessary constructors

Summary: This is continuing the work done by 27b22f1 It's just cleaning up some unnecessary constructors. The most important change is removing Block::Block(const BlockContents& contents) constructor. It was only used from the unit test. Test Plan: compiles Reviewers: sdong, yhchiang, ljin Reviewed By: ljin Subscribers: leveldb Differential Revision: https://reviews.facebook.net/D23547
facebook · Sep 17, 2014 · ff76895 · ff76895
1 parent feadb9d
commit ff76895
Show file tree

Hide file tree

Showing 6 changed files with 34 additions and 44 deletions.
diff --git a/table/block.cc b/table/block.cc
@@ -297,8 +297,10 @@ uint32_t Block::NumRestarts() const {
   return DecodeFixed32(data_ + size_ - sizeof(uint32_t));
 }
 
-Block::Block(const BlockContents& contents)
-    : data_(contents.data.data()), size_(contents.data.size()) {
+Block::Block(BlockContents&& contents)
+    : contents_(std::move(contents)),
+      data_(contents_.data.data()),
+      size_(contents_.data.size()) {
   if (size_ < sizeof(uint32_t)) {
     size_ = 0;  // Error marker
   } else {
@@ -311,10 +313,6 @@ Block::Block(const BlockContents& contents)
   }
 }
 
-Block::Block(BlockContents&& contents) : Block(contents) {
-  contents_ = std::move(contents);
-}
-
 Iterator* Block::NewIterator(
     const Comparator* cmp, BlockIter* iter, bool total_order_seek) {
   if (size_ < 2*sizeof(uint32_t)) {

diff --git a/table/block.h b/table/block.h
@@ -31,7 +31,6 @@ class Block {
  public:
   // Initialize the block with the specified contents.
   explicit Block(BlockContents&& contents);
-  explicit Block(const BlockContents& contents);
 
   ~Block() = default;
 
@@ -66,8 +65,8 @@ class Block {
 
  private:
   BlockContents contents_;
-  const char* data_;
-  size_t size_;
+  const char* data_;            // contents_.data.data()
+  size_t size_;                 // contents_.data.size()
   uint32_t restart_offset_;     // Offset in data_ of restart array
   std::unique_ptr<BlockHashIndex> hash_index_;
   std::unique_ptr<BlockPrefixIndex> prefix_index_;

diff --git a/table/block_based_filter_block.cc b/table/block_based_filter_block.cc
@@ -137,32 +137,26 @@ void BlockBasedFilterBlockBuilder::GenerateFilter() {
 
 BlockBasedFilterBlockReader::BlockBasedFilterBlockReader(
     const SliceTransform* prefix_extractor,
-    const BlockBasedTableOptions& table_opt, const Slice& contents)
+    const BlockBasedTableOptions& table_opt, BlockContents&& contents)
     : policy_(table_opt.filter_policy.get()),
       prefix_extractor_(prefix_extractor),
       whole_key_filtering_(table_opt.whole_key_filtering),
       data_(nullptr),
       offset_(nullptr),
       num_(0),
-      base_lg_(0) {
+      base_lg_(0),
+      contents_(std::move(contents)) {
   assert(policy_);
-  size_t n = contents.size();
+  size_t n = contents_.data.size();
   if (n < 5) return;  // 1 byte for base_lg_ and 4 for start of offset array
-  base_lg_ = contents[n - 1];
-  uint32_t last_word = DecodeFixed32(contents.data() + n - 5);
+  base_lg_ = contents_.data[n - 1];
+  uint32_t last_word = DecodeFixed32(contents_.data.data() + n - 5);
   if (last_word > n - 5) return;
-  data_ = contents.data();
+  data_ = contents_.data.data();
   offset_ = data_ + last_word;
   num_ = (n - 5 - last_word) / 4;
 }
 
-BlockBasedFilterBlockReader::BlockBasedFilterBlockReader(
-    const SliceTransform* prefix_extractor,
-    const BlockBasedTableOptions& table_opt, BlockContents&& contents)
-    : BlockBasedFilterBlockReader(prefix_extractor, table_opt, contents.data) {
-  contents_ = std::move(contents);
-}
-
 bool BlockBasedFilterBlockReader::KeyMayMatch(const Slice& key,
                                               uint64_t block_offset) {
   assert(block_offset != kNotValid);

diff --git a/table/block_based_filter_block.h b/table/block_based_filter_block.h
@@ -72,9 +72,6 @@ class BlockBasedFilterBlockBuilder : public FilterBlockBuilder {
 class BlockBasedFilterBlockReader : public FilterBlockReader {
  public:
   // REQUIRES: "contents" and *policy must stay live while *this is live.
-  BlockBasedFilterBlockReader(const SliceTransform* prefix_extractor,
-                              const BlockBasedTableOptions& table_opt,
-                              const Slice& contents);
   BlockBasedFilterBlockReader(const SliceTransform* prefix_extractor,
                               const BlockBasedTableOptions& table_opt,
                               BlockContents&& contents);

diff --git a/table/block_based_filter_block_test.cc b/table/block_based_filter_block_test.cc
@@ -55,9 +55,9 @@ class FilterBlockTest {
 
 TEST(FilterBlockTest, EmptyBuilder) {
   BlockBasedFilterBlockBuilder builder(nullptr, table_options_);
-  Slice block = builder.Finish();
-  ASSERT_EQ("\\x00\\x00\\x00\\x00\\x0b", EscapeString(block));
-  BlockBasedFilterBlockReader reader(nullptr, table_options_, block);
+  BlockContents block(builder.Finish(), false, kNoCompression);
+  ASSERT_EQ("\\x00\\x00\\x00\\x00\\x0b", EscapeString(block.data));
+  BlockBasedFilterBlockReader reader(nullptr, table_options_, std::move(block));
   ASSERT_TRUE(reader.KeyMayMatch("foo", 0));
   ASSERT_TRUE(reader.KeyMayMatch("foo", 100000));
 }
@@ -72,8 +72,8 @@ TEST(FilterBlockTest, SingleChunk) {
   builder.Add("box");
   builder.StartBlock(300);
   builder.Add("hello");
-  Slice block = builder.Finish();
-  BlockBasedFilterBlockReader reader(nullptr, table_options_, block);
+  BlockContents block(builder.Finish(), false, kNoCompression);
+  BlockBasedFilterBlockReader reader(nullptr, table_options_, std::move(block));
   ASSERT_TRUE(reader.KeyMayMatch("foo", 100));
   ASSERT_TRUE(reader.KeyMayMatch("bar", 100));
   ASSERT_TRUE(reader.KeyMayMatch("box", 100));
@@ -103,8 +103,8 @@ TEST(FilterBlockTest, MultiChunk) {
   builder.Add("box");
   builder.Add("hello");
 
-  Slice block = builder.Finish();
-  BlockBasedFilterBlockReader reader(nullptr, table_options_, block);
+  BlockContents block(builder.Finish(), false, kNoCompression);
+  BlockBasedFilterBlockReader reader(nullptr, table_options_, std::move(block));
 
   // Check first filter
   ASSERT_TRUE(reader.KeyMayMatch("foo", 0));
@@ -147,10 +147,10 @@ class BlockBasedFilterBlockTest {
 TEST(BlockBasedFilterBlockTest, BlockBasedEmptyBuilder) {
   FilterBlockBuilder* builder = new BlockBasedFilterBlockBuilder(
       nullptr, table_options_);
-  Slice block = builder->Finish();
-  ASSERT_EQ("\\x00\\x00\\x00\\x00\\x0b", EscapeString(block));
+  BlockContents block(builder->Finish(), false, kNoCompression);
+  ASSERT_EQ("\\x00\\x00\\x00\\x00\\x0b", EscapeString(block.data));
   FilterBlockReader* reader = new BlockBasedFilterBlockReader(
-      nullptr, table_options_, block);
+      nullptr, table_options_, std::move(block));
   ASSERT_TRUE(reader->KeyMayMatch("foo", 0));
   ASSERT_TRUE(reader->KeyMayMatch("foo", 100000));
 
@@ -169,9 +169,9 @@ TEST(BlockBasedFilterBlockTest, BlockBasedSingleChunk) {
   builder->Add("box");
   builder->StartBlock(300);
   builder->Add("hello");
-  Slice block = builder->Finish();
+  BlockContents block(builder->Finish(), false, kNoCompression);
   FilterBlockReader* reader = new BlockBasedFilterBlockReader(
-      nullptr, table_options_, block);
+      nullptr, table_options_, std::move(block));
   ASSERT_TRUE(reader->KeyMayMatch("foo", 100));
   ASSERT_TRUE(reader->KeyMayMatch("bar", 100));
   ASSERT_TRUE(reader->KeyMayMatch("box", 100));
@@ -205,9 +205,9 @@ TEST(BlockBasedFilterBlockTest, BlockBasedMultiChunk) {
   builder->Add("box");
   builder->Add("hello");
 
-  Slice block = builder->Finish();
+  BlockContents block(builder->Finish(), false, kNoCompression);
   FilterBlockReader* reader = new BlockBasedFilterBlockReader(
-      nullptr, table_options_, block);
+      nullptr, table_options_, std::move(block));
 
   // Check first filter
   ASSERT_TRUE(reader->KeyMayMatch("foo", 0));

diff --git a/table/block_test.cc b/table/block_test.cc
@@ -146,13 +146,15 @@ BlockContents GetBlockContents(std::unique_ptr<BlockBuilder> *builder,
   return contents;
 }
 
-void CheckBlockContents(const BlockContents &contents, const int max_key,
+void CheckBlockContents(BlockContents contents, const int max_key,
                         const std::vector<std::string> &keys,
                         const std::vector<std::string> &values) {
   const size_t prefix_size = 6;
   // create block reader
-  Block reader1(contents);
-  Block reader2(contents);
+  BlockContents contents_ref(contents.data, contents.cachable,
+                             contents.compression_type);
+  Block reader1(std::move(contents));
+  Block reader2(std::move(contents_ref));
 
   std::unique_ptr<const SliceTransform> prefix_extractor(
       NewFixedPrefixTransform(prefix_size));
@@ -210,7 +212,7 @@ TEST(BlockTest, SimpleIndexHash) {
   std::unique_ptr<BlockBuilder> builder;
   auto contents = GetBlockContents(&builder, keys, values);
 
-  CheckBlockContents(contents, kMaxKey, keys, values);
+  CheckBlockContents(std::move(contents), kMaxKey, keys, values);
 }
 
 TEST(BlockTest, IndexHashWithSharedPrefix) {
@@ -229,7 +231,7 @@ TEST(BlockTest, IndexHashWithSharedPrefix) {
   std::unique_ptr<BlockBuilder> builder;
   auto contents = GetBlockContents(&builder, keys, values, kPrefixGroup);
 
-  CheckBlockContents(contents, kMaxKey, keys, values);
+  CheckBlockContents(std::move(contents), kMaxKey, keys, values);
 }
 
 }  // namespace rocksdb