diff --git a/include/map.h b/include/map.h index 0c249cb..a2ea09c 100644 --- a/include/map.h +++ b/include/map.h @@ -452,7 +452,7 @@ class map ++it; } } - return map(copy); + return map(std::move(copy)); } // Executes the given operation for each key/value pair in the map. The operation must not @@ -543,7 +543,7 @@ class map { auto copy(m_map); copy.erase(key); - return map(copy); + return map(std::move(copy)); } // Inserts a key/value pair in place (mutating). If an equivalent key already exists, the @@ -583,7 +583,7 @@ class map { auto copy(m_map); copy.insert(std::make_pair(key, value)); - return map(copy); + return map(std::move(copy)); } // Returns a copy of this instance with the key/value pair inserted (non-mutating). If an @@ -592,7 +592,7 @@ class map { auto copy(m_map); copy.insert(element); - return map(copy); + return map(std::move(copy)); } // Removes all key/value pairs from the map (mutating) @@ -651,8 +651,9 @@ class map return m_map.end(); } - // Returns a reference to the value that is mapped to a key, - // assuming that such key already exists. + // Returns a reference to the value that is mapped to a key, which must already exist. + // If the key is not present the program calls std::abort() (in both debug and release + // builds). Define FCPP_NO_PRECONDITION_CHECKS to disable the check. const TValue& operator[](const TKey& key) const { const auto it = m_map.find(key); diff --git a/include/set.h b/include/set.h index 6630b9d..ce75fab 100644 --- a/include/set.h +++ b/include/set.h @@ -518,7 +518,7 @@ namespace fcpp { other.begin(), other.end(), std::inserter(diff, diff.begin())); - return set(diff); + return set(std::move(diff)); } [[nodiscard]] set difference_with(const std::set& other) const @@ -545,7 +545,7 @@ namespace fcpp { other.begin(), other.end(), std::inserter(combined, combined.begin())); - return set(combined); + return set(std::move(combined)); } [[nodiscard]] set union_with(const std::set& other) const @@ -572,7 +572,7 @@ namespace fcpp { other.begin(), other.end(), std::inserter(intersection, intersection.begin())); - return set(intersection); + return set(std::move(intersection)); } [[nodiscard]] set intersect_with(const std::set& other) const @@ -656,7 +656,7 @@ namespace fcpp { for (const auto& key : m_set) { transformed_set.insert(transform(key)); } - return set(transformed_set); + return set(std::move(transformed_set)); } // Returns true if all keys match the predicate (return true) @@ -830,7 +830,7 @@ namespace fcpp { copy.insert(*it); } } - return set(copy); + return set(std::move(copy)); } #ifdef CPP17_AVAILABLE @@ -928,7 +928,7 @@ namespace fcpp { for_each([&vec](const TKey& key){ vec.insert_back(key); }); - return std::move(vec); + return vec; } // Removes an element from the set, if it exists, potentially changing the set's contents (mutating) @@ -958,7 +958,7 @@ namespace fcpp { { auto copy(m_set); copy.erase(element); - return set(copy); + return set(std::move(copy)); } // Inserts an element in the set, if it does not already exist, potentially changing the set's contents (mutating) @@ -988,7 +988,7 @@ namespace fcpp { { auto copy(m_set); copy.insert(element); - return set(copy); + return set(std::move(copy)); } // Removes all keys from the set (mutating) @@ -1089,7 +1089,8 @@ namespace fcpp { } // Returns a copy of the key at the given sorted position. - // Bounds checking (assert) is enabled for debug builds. + // Bounds checking is always enabled (in both debug and release builds): an out-of-bounds + // index calls std::abort(). Define FCPP_NO_PRECONDITION_CHECKS to disable the check. // Performance is O(n), so be careful for performance critical code sections. TKey operator[](size_t index) { @@ -1100,7 +1101,8 @@ namespace fcpp { } // Returns a copy of the key at the given sorted position. - // Bounds checking (assert) is enabled for debug builds. + // Bounds checking is always enabled (in both debug and release builds): an out-of-bounds + // index calls std::abort(). Define FCPP_NO_PRECONDITION_CHECKS to disable the check. // Performance is O(n), so be careful for performance critical code sections. TKey operator[](size_t index) const { @@ -1180,7 +1182,7 @@ namespace fcpp { for (; it1 != end() && it2 != set_end; ++it1, ++it2) { combined_set.insert({*it1, *it2}); } - return set>(combined_set); + return set>(std::move(combined_set)); } }; diff --git a/include/vector.h b/include/vector.h index f1b955c..5d7f2bc 100644 --- a/include/vector.h +++ b/include/vector.h @@ -454,7 +454,7 @@ namespace fcpp { m_vector.end(), std::back_inserter(transformed_vector), std::forward(transform)); - return vector(transformed_vector); + return vector(std::move(transformed_vector)); } #ifdef PARALLEL_ALGORITHM_AVAILABLE @@ -470,7 +470,7 @@ namespace fcpp { m_vector.cend(), transformed_vector.begin(), std::forward(transform)); - return vector(transformed_vector); + return vector(std::move(transformed_vector)); } #endif @@ -704,7 +704,7 @@ namespace fcpp { m_vector.end(), std::back_inserter(filtered_vector), std::forward(predicate_to_keep)); - return vector(filtered_vector); + return vector(std::move(filtered_vector)); } #ifdef PARALLEL_ALGORITHM_AVAILABLE @@ -726,7 +726,7 @@ namespace fcpp { m_vector.end(), std::back_inserter(filtered_vector), std::forward(predicate_to_keep)); - return vector(filtered_vector); + return vector(std::move(filtered_vector)); #endif } #endif @@ -999,7 +999,7 @@ namespace fcpp { std::sort(sorted_vector.begin(), sorted_vector.end(), std::forward(comparison_predicate)); - return vector(sorted_vector); + return vector(std::move(sorted_vector)); } #ifdef PARALLEL_ALGORITHM_AVAILABLE @@ -1013,7 +1013,7 @@ namespace fcpp { sorted_vector.begin(), sorted_vector.end(), std::forward(comparison_predicate)); - return fcpp::vector(sorted_vector); + return fcpp::vector(std::move(sorted_vector)); } #endif @@ -1192,7 +1192,7 @@ namespace fcpp { assert_smaller_size(index); auto copy(m_vector); copy.erase(copy.begin() + index); - return vector(copy); + return vector(std::move(copy)); } // Removes the last element, if present (mutating) @@ -1221,7 +1221,7 @@ namespace fcpp { { auto copy(m_vector); copy.pop_back(); - return vector(copy); + return vector(std::move(copy)); } // Removes the first element, if present (mutating) @@ -1294,7 +1294,7 @@ namespace fcpp { auto shorter_vector(m_vector); shorter_vector.erase(shorter_vector.begin() + range.start, shorter_vector.begin() + range.start + range.count); - return vector(shorter_vector); + return vector(std::move(shorter_vector)); } // Inserts an element at the given index, therefore changing the vector's contents (mutating) @@ -1325,7 +1325,7 @@ namespace fcpp { assert_smaller_or_equal_size(index); auto copy(m_vector); copy.insert(copy.begin() + index, element); - return vector(copy); + return vector(std::move(copy)); } // Inserts a range of elements starting at the given index, therefore changing the vector's contents (mutating) @@ -1422,7 +1422,7 @@ namespace fcpp { // numbers -> fcpp::vector({1, 4, 2, 5, 8, 3, 1, 7, 1, 18}); vector& insert_back(T value) { - m_vector.push_back(value); + m_vector.push_back(std::move(value)); return *this; } @@ -1450,8 +1450,8 @@ namespace fcpp { [[nodiscard]] vector inserting_back(T value) const { auto augmented_vector(m_vector); - augmented_vector.push_back(value); - return vector(augmented_vector); + augmented_vector.push_back(std::move(value)); + return vector(std::move(augmented_vector)); } // Makes a copy of the vector, inserts value at the beginning of the copy and returns the copy (non-mutating) @@ -1824,7 +1824,8 @@ namespace fcpp { } // Returns a reference to the element in the given index, allowing subscripting and value editing. - // Bounds checking (assert) is enabled for debug builds. + // Bounds checking is always enabled (in both debug and release builds): an out-of-bounds + // index calls std::abort(). Define FCPP_NO_PRECONDITION_CHECKS to disable the check. T& operator[](size_t index) { assert_smaller_size(index); @@ -1832,7 +1833,8 @@ namespace fcpp { } // Returns a constant reference to the element in the given index, allowing subscripting. - // Bounds checking (assert) is enabled for debug builds. + // Bounds checking is always enabled (in both debug and release builds): an out-of-bounds + // index calls std::abort(). Define FCPP_NO_PRECONDITION_CHECKS to disable the check. const T& operator[](size_t index) const { assert_smaller_size(index); @@ -1912,7 +1914,7 @@ namespace fcpp { augmented_vector.insert(augmented_vector.end(), vec_begin, vec_end); - return vector(augmented_vector); + return vector(std::move(augmented_vector)); } #ifdef CPP17_AVAILABLE @@ -1928,7 +1930,7 @@ namespace fcpp { augmented_vector.insert(augmented_vector.begin(), vec_begin, vec_end); - return vector(augmented_vector); + return vector(std::move(augmented_vector)); } #ifdef CPP17_AVAILABLE @@ -2030,7 +2032,7 @@ namespace fcpp { std::copy(vec_begin, vec_end, replaced_vector.begin() + index); - return vector(replaced_vector); + return vector(std::move(replaced_vector)); } void assert_smaller_size(size_t index) const diff --git a/tests/set_test.cc b/tests/set_test.cc index 3a83a3f..d0ee744 100644 --- a/tests/set_test.cc +++ b/tests/set_test.cc @@ -1260,3 +1260,15 @@ TEST(SetTest, LazyIntersectionWithFunctionalSetPreservesComparatorState) EXPECT_EQ(4, intersection[0]); EXPECT_EQ(2, intersection[1]); } + +TEST(SetTest, NonMutatingReturnMovesInsteadOfCopies) +{ + const set set_under_test(std::vector({counted(3), counted(1), counted(2)})); + counted::reset(); + const auto unchanged = set_under_test.removing(counted(999)); + // The single unavoidable copy is the working copy of the underlying std::set + // (3 elements). The returned wrapper must move that copy, not copy it again. + // Before the fix this was 2 * 3 = 6. + EXPECT_EQ(3, counted::copy_count()); + EXPECT_EQ(3u, unchanged.size()); +} diff --git a/tests/test_types.h b/tests/test_types.h index 70b66c2..914e902 100644 --- a/tests/test_types.h +++ b/tests/test_types.h @@ -80,3 +80,28 @@ struct person_comparator { return a < b; } }; + +// A type that counts how many times it is copy- vs move-constructed, used to verify +// that non-mutating operations move their internal containers instead of copying them. +// The counters are function-local statics so the type stays header-only across all +// test translation units (no separate .cc and no C++17 inline-variable requirement). +struct counted +{ + static int& copy_count() { static int count = 0; return count; } + static int& move_count() { static int count = 0; return count; } + static void reset() { copy_count() = 0; move_count() = 0; } + + counted() : value(0) {} + counted(int value) : value(value) {} + + counted(const counted& other) : value(other.value) { ++copy_count(); } + counted(counted&& other) noexcept : value(other.value) { ++move_count(); } + + counted& operator=(const counted& other) { value = other.value; return *this; } + counted& operator=(counted&& other) noexcept { value = other.value; return *this; } + + int value; + + bool operator==(const counted& other) const { return value == other.value; } + bool operator<(const counted& other) const { return value < other.value; } +}; diff --git a/tests/vector_test.cc b/tests/vector_test.cc index f42f3ce..b55264c 100644 --- a/tests/vector_test.cc +++ b/tests/vector_test.cc @@ -1687,4 +1687,29 @@ TEST(VectorTest, LazyZipWithLazyVectorDifferentSizesThrows) EXPECT_DEATH({ const auto zipped_vector = vector_under_test.lazy().zip(names).get(); }, ""); } +TEST(VectorTest, NonMutatingReturnMovesInsteadOfCopies) +{ + const vector vector_under_test({counted(3), counted(1), counted(4), counted(2)}); + counted::reset(); + const auto shorter = vector_under_test.removing_back(); + // The single unavoidable copy is the working copy of the underlying std::vector + // (4 elements). The returned wrapper must move that copy, not copy it again. + // Before the fix this was 2 * 4 - 1 = 7. + EXPECT_EQ(4, counted::copy_count()); + EXPECT_EQ(3u, shorter.size()); +} + +TEST(VectorTest, InsertBackMovesElementInsteadOfCopyingTwice) +{ + vector vector_under_test; + vector_under_test.reserve(10); + const counted element(5); + counted::reset(); + vector_under_test.insert_back(element); + // The lvalue is copied once into the by-value parameter, then moved into storage. + // Before the fix the parameter was copied a second time (2 copies, 0 moves). + EXPECT_EQ(1, counted::copy_count()); + EXPECT_EQ(1, counted::move_count()); +} + #pragma warning( pop )