Avoid unnecessary container copies in non-mutating operations (#30)

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);

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<TKey, TCompare>& 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<TKey, TCompare>& 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<TKey, TCompare>& other) const
@@ -656,7 +656,7 @@ namespace fcpp {
             for (const auto& key : m_set) {
                 transformed_set.insert(transform(key));
             }
-            return set<UKey, UCompare>(transformed_set);
+            return set<UKey, UCompare>(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<std::pair<TKey, UKey>>(combined_set);
+            return set<std::pair<TKey, UKey>>(std::move(combined_set));
         }
     };
 

include/vector.h

@@ -454,7 +454,7 @@ namespace fcpp {
                            m_vector.end(),
                            std::back_inserter(transformed_vector),
                            std::forward<Transform>(transform));
-            return vector<U>(transformed_vector);
+            return vector<U>(std::move(transformed_vector));
         }
 
 #ifdef PARALLEL_ALGORITHM_AVAILABLE
@@ -470,7 +470,7 @@ namespace fcpp {
                            m_vector.cend(),
                            transformed_vector.begin(),
                            std::forward<Transform>(transform));
-            return vector<U>(transformed_vector);
+            return vector<U>(std::move(transformed_vector));
         }
 #endif
 
@@ -704,7 +704,7 @@ namespace fcpp {
                          m_vector.end(),
                          std::back_inserter(filtered_vector),
                          std::forward<Callable>(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<Callable>(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<Sortable>(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<Sortable>(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,15 +1824,17 @@ 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);
             return m_vector[index];
         }
 
         // 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

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<counted> set_under_test(std::vector<counted>({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());
+}

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; }
+};

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<counted> 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<counted> 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 )