From commits-return-6477-archive-asf-public=cust-asf.ponee.io@kudu.apache.org Fri Oct 5 20:39:53 2018 Return-Path: X-Original-To: archive-asf-public@cust-asf.ponee.io Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by mx-eu-01.ponee.io (Postfix) with SMTP id E5618180649 for ; Fri, 5 Oct 2018 20:39:52 +0200 (CEST) Received: (qmail 91079 invoked by uid 500); 5 Oct 2018 18:39:52 -0000 Mailing-List: contact commits-help@kudu.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@kudu.apache.org Delivered-To: mailing list commits@kudu.apache.org Received: (qmail 91070 invoked by uid 99); 5 Oct 2018 18:39:52 -0000 Received: from git1-us-west.apache.org (HELO git1-us-west.apache.org) (140.211.11.23) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 05 Oct 2018 18:39:52 +0000 Received: by git1-us-west.apache.org (ASF Mail Server at git1-us-west.apache.org, from userid 33) id E4570E00AA; Fri, 5 Oct 2018 18:39:51 +0000 (UTC) Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit From: wdberkeley@apache.org To: commits@kudu.apache.org Message-Id: <00cf381c2e064288815492f252dc6297@git.apache.org> X-Mailer: ASF-Git Admin Mailer Subject: kudu git commit: Move constructor and assignment operator for Schema Date: Fri, 5 Oct 2018 18:39:51 +0000 (UTC) Repository: kudu Updated Branches: refs/heads/master cf1b1f42c -> b848488d9 Move constructor and assignment operator for Schema This adds a move constructor and move assignment operator to Schema. I removed Schema::swap. It was only used in tests, and any potential use of it should be adequately fulfilled by one of moves, copies, or Schema::Reset. Change-Id: I6c827c84657875be4cf2bc565db03a686849d8e2 Reviewed-on: http://gerrit.cloudera.org:8080/11593 Reviewed-by: Adar Dembo Tested-by: Kudu Jenkins Reviewed-by: Alexey Serbin Project: http://git-wip-us.apache.org/repos/asf/kudu/repo Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/b848488d Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/b848488d Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/b848488d Branch: refs/heads/master Commit: b848488d9bd2efcfc63ddc073c3addece33793b6 Parents: cf1b1f4 Author: Will Berkeley Authored: Sun Sep 30 10:17:21 2018 -0700 Committer: Will Berkeley Committed: Fri Oct 5 18:38:54 2018 +0000 ---------------------------------------------------------------------- src/kudu/common/id_mapping.h | 13 ------ src/kudu/common/schema-test.cc | 80 ++++++++++++++++++++++++++++--------- src/kudu/common/schema.cc | 49 ++++++++++++++++++----- src/kudu/common/schema.h | 29 +++++++------- 4 files changed, 115 insertions(+), 56 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/kudu/blob/b848488d/src/kudu/common/id_mapping.h ---------------------------------------------------------------------- diff --git a/src/kudu/common/id_mapping.h b/src/kudu/common/id_mapping.h index a5d8289..f762b75 100644 --- a/src/kudu/common/id_mapping.h +++ b/src/kudu/common/id_mapping.h @@ -64,13 +64,6 @@ class IdMapping { clear(); } - explicit IdMapping(const IdMapping& other) - : mask_(other.mask_), - entries_(other.entries_) { - } - - ~IdMapping() {} - void clear() { ClearMap(&entries_); } @@ -84,12 +77,6 @@ class IdMapping { other.entries_.swap(entries_); } - IdMapping& operator=(const IdMapping& other) { - mask_ = other.mask_; - entries_ = other.entries_; - return *this; - } - int operator[](int key) const { return get(key); } http://git-wip-us.apache.org/repos/asf/kudu/blob/b848488d/src/kudu/common/schema-test.cc ---------------------------------------------------------------------- diff --git a/src/kudu/common/schema-test.cc b/src/kudu/common/schema-test.cc index 503ebea..87ba59a 100644 --- a/src/kudu/common/schema-test.cc +++ b/src/kudu/common/schema-test.cc @@ -23,6 +23,7 @@ #include #include // IWYU pragma: keep #include +#include #include #include // IWYU pragma: keep @@ -102,6 +103,65 @@ TEST_F(TestSchema, TestSchema) { EXPECT_EQ("uint32 NULLABLE", schema.column(1).TypeToString()); } +TEST_F(TestSchema, TestCopyAndMove) { + auto check_schema = [](const Schema& schema) { + ASSERT_EQ(sizeof(Slice) + sizeof(uint32_t) + sizeof(int32_t), + schema.byte_size()); + ASSERT_EQ(3, schema.num_columns()); + ASSERT_EQ(0, schema.column_offset(0)); + ASSERT_EQ(sizeof(Slice), schema.column_offset(1)); + + EXPECT_EQ("Schema [\n" + "\tprimary key (key),\n" + "\tkey[string NOT NULL],\n" + "\tuint32val[uint32 NULLABLE],\n" + "\tint32val[int32 NOT NULL]\n" + "]", + schema.ToString()); + EXPECT_EQ("key[string NOT NULL]", schema.column(0).ToString()); + EXPECT_EQ("uint32 NULLABLE", schema.column(1).TypeToString()); + }; + + ColumnSchema col1("key", STRING); + ColumnSchema col2("uint32val", UINT32, true); + ColumnSchema col3("int32val", INT32); + + vector cols = { col1, col2, col3 }; + Schema schema(cols, 1); + check_schema(schema); + + // Check copy- and move-assignment. + Schema moved_schema; + { + Schema copied_schema = schema; + check_schema(copied_schema); + ASSERT_TRUE(copied_schema.Equals(schema)); + + // Move-assign to 'moved_to_schema' from 'copied_schema' and then let + // 'copied_schema' go out of scope to make sure none of 'moved_to_schema''s + // resources are incorrectly freed. + moved_schema = std::move(copied_schema); + + // 'copied_schema' is moved from so it should still be valid to call + // ToString(), though we can't expect any particular result. + NO_FATALS(copied_schema.ToString()); // NOLINT(*) + } + check_schema(moved_schema); + ASSERT_TRUE(moved_schema.Equals(schema)); + + // Check copy- and move-construction. + { + Schema copied_schema(schema); + check_schema(copied_schema); + ASSERT_TRUE(copied_schema.Equals(schema)); + + Schema moved_schema(std::move(copied_schema)); + NO_FATALS(copied_schema.ToString()); // NOLINT(*) + check_schema(moved_schema); + ASSERT_TRUE(moved_schema.Equals(schema)); + } +} + // Test basic functionality of Schema definition with decimal columns TEST_F(TestSchema, TestSchemaWithDecimal) { ColumnSchema col1("key", STRING); @@ -163,21 +223,6 @@ TEST_F(TestSchema, TestSchemaEqualsWithDecimal) { EXPECT_FALSE(schema_18_10.Equals(schema_17_9)); } -TEST_F(TestSchema, TestSwap) { - Schema schema1({ ColumnSchema("col1", STRING), - ColumnSchema("col2", STRING), - ColumnSchema("col3", UINT32) }, - 2); - Schema schema2({ ColumnSchema("col3", UINT32), - ColumnSchema("col2", STRING) }, - 1); - schema1.swap(schema2); - ASSERT_EQ(2, schema1.num_columns()); - ASSERT_EQ(1, schema1.num_key_columns()); - ASSERT_EQ(3, schema2.num_columns()); - ASSERT_EQ(2, schema2.num_key_columns()); -} - TEST_F(TestSchema, TestColumnSchemaEquals) { Slice default_str("read-write default"); ColumnSchema col1("key", STRING); @@ -233,11 +278,10 @@ TEST_F(TestSchema, TestReset) { 1)); ASSERT_TRUE(schema.initialized()); - // Swap the initialized schema with an uninitialized one. + // Move an uninitialized schema into the initialized schema. Schema schema2; - schema2.swap(schema); + schema = std::move(schema2); ASSERT_FALSE(schema.initialized()); - ASSERT_TRUE(schema2.initialized()); } // Test for KUDU-943, a bug where we suspected that Variant didn't behave http://git-wip-us.apache.org/repos/asf/kudu/blob/b848488d/src/kudu/common/schema.cc ---------------------------------------------------------------------- diff --git a/src/kudu/common/schema.cc b/src/kudu/common/schema.cc index c999e6a..b15652e 100644 --- a/src/kudu/common/schema.cc +++ b/src/kudu/common/schema.cc @@ -141,8 +141,7 @@ size_t ColumnSchema::memory_footprint_including_this() const { Schema::Schema(const Schema& other) : name_to_index_bytes_(0), - // TODO(adar): C++11 provides a single-arg constructor - name_to_index_(10, + name_to_index_(/*bucket_count=*/10, NameToIndexMap::hasher(), NameToIndexMap::key_equal(), NameToIndexMapAllocator(&name_to_index_bytes_)) { @@ -175,15 +174,45 @@ void Schema::CopyFrom(const Schema& other) { has_nullables_ = other.has_nullables_; } -void Schema::swap(Schema& other) { - std::swap(num_key_columns_, other.num_key_columns_); - cols_.swap(other.cols_); - col_ids_.swap(other.col_ids_); - col_offsets_.swap(other.col_offsets_); - name_to_index_.swap(other.name_to_index_); - id_to_index_.swap(other.id_to_index_); - std::swap(has_nullables_, other.has_nullables_); +Schema::Schema(Schema&& other) noexcept + : cols_(std::move(other.cols_)), + num_key_columns_(other.num_key_columns_), + col_ids_(std::move(other.col_ids_)), + col_offsets_(std::move(other.col_offsets_)), + name_to_index_bytes_(0), + name_to_index_(/*bucket_count=*/10, + NameToIndexMap::hasher(), + NameToIndexMap::key_equal(), + NameToIndexMapAllocator(&name_to_index_bytes_)), + id_to_index_(std::move(other.id_to_index_)), + has_nullables_(other.has_nullables_) { + // 'name_to_index_' uses a customer allocator which holds a pointer to + // 'name_to_index_bytes_'. swap() will swap the contents but not the + // allocators; std::move will move the allocator[1], which will mean the moved + // to value will be holding a pointer to the moved-from's member, which is + // wrong and will likely lead to use-after-free. The 'name_to_index_bytes_' + // values should also be swapped so both schemas end up in a valid state. + // + // [1] STLCountingAllocator::propagate_on_container_move_assignment::value + // is std::true_type, which it inherits from the default Allocator. std::swap(name_to_index_bytes_, other.name_to_index_bytes_); + name_to_index_.swap(other.name_to_index_); +} + +Schema& Schema::operator=(Schema&& other) noexcept { + if (&other != this) { + cols_ = std::move(other.cols_); + num_key_columns_ = other.num_key_columns_; + col_ids_ = std::move(other.col_ids_); + col_offsets_ = std::move(other.col_offsets_); + id_to_index_ = std::move(other.id_to_index_); + has_nullables_ = other.has_nullables_; + + // See the comment in the move constructor implementation for why we swap. + std::swap(name_to_index_bytes_, other.name_to_index_bytes_); + name_to_index_.swap(other.name_to_index_); + } + return *this; } Status Schema::Reset(const vector& cols, http://git-wip-us.apache.org/repos/asf/kudu/blob/b848488d/src/kudu/common/schema.h ---------------------------------------------------------------------- diff --git a/src/kudu/common/schema.h b/src/kudu/common/schema.h index b5e2003..449f3cd 100644 --- a/src/kudu/common/schema.h +++ b/src/kudu/common/schema.h @@ -400,10 +400,10 @@ class ColumnSchema { // which prefix of columns makes up the primary key. // // Note that, while Schema is copyable and assignable, it is a complex -// object that is not inexpensive to copy. You should generally prefer -// passing by pointer or reference, and functions that create new +// object that is not inexpensive to copy. Prefer move semantics when +// possible, or pass by pointer or reference. Functions that create new // Schemas should generally prefer taking a Schema pointer and using -// Schema::swap() or Schema::Reset() rather than returning by value. +// Schema::Reset() rather than returning by value. class Schema { public: @@ -412,8 +412,10 @@ class Schema { Schema() : num_key_columns_(0), name_to_index_bytes_(0), - // TODO: C++11 provides a single-arg constructor - name_to_index_(10, + // TODO(wdberkeley): C++11 provides a single-argument constructor, but + // it's not supported in GCC < 4.9. This (and the other occurrences here + // and in schema.cc) can be fixed if we adopt C++14 or a later standard. + name_to_index_(/*bucket_count=*/10, NameToIndexMap::hasher(), NameToIndexMap::key_equal(), NameToIndexMapAllocator(&name_to_index_bytes_)), @@ -423,9 +425,8 @@ class Schema { Schema(const Schema& other); Schema& operator=(const Schema& other); - // TODO(todd) implement a move constructor - - void swap(Schema& other); // NOLINT(build/include_what_you_use) + Schema(Schema&& other) noexcept; + Schema& operator=(Schema&& other) noexcept; void CopyFrom(const Schema& other); @@ -438,8 +439,7 @@ class Schema { Schema(const std::vector& cols, int key_columns) : name_to_index_bytes_(0), - // TODO: C++11 provides a single-arg constructor - name_to_index_(10, + name_to_index_(/*bucket_count=*/10, NameToIndexMap::hasher(), NameToIndexMap::key_equal(), NameToIndexMapAllocator(&name_to_index_bytes_)) { @@ -456,8 +456,7 @@ class Schema { const std::vector& ids, int key_columns) : name_to_index_bytes_(0), - // TODO: C++11 provides a single-arg constructor - name_to_index_(10, + name_to_index_(/*bucket_count=*/10, NameToIndexMap::hasher(), NameToIndexMap::key_equal(), NameToIndexMapAllocator(&name_to_index_bytes_)) { @@ -857,7 +856,6 @@ class Schema { size_t memory_footprint_including_this() const; private: - // Return a stringified version of the first 'num_columns' columns of the // row. template @@ -906,8 +904,9 @@ class Schema { // Cached indicator whether any columns are nullable. bool has_nullables_; - // NOTE: if you add more members, make sure to add the appropriate - // code to swap() and CopyFrom() as well to prevent subtle bugs. + // NOTE: if you add more members, make sure to add the appropriate code to + // CopyFrom() and the move constructor and assignment operator as well, to + // prevent subtle bugs. }; // Helper used for schema creation/editing.