Avoid unnecessary container copies in non-mutating operations (#30)#35
Merged
Conversation
) Move local containers into the returned wrapper instead of copying them. Many non-mutating operations (filtered, sorted, removing_*, inserting_*, replace/replacing_range, map, set difference/union/intersect/map/zip, and the map removing/inserting/filtered helpers) built a local std::vector/std::set/ std::map and passed it as an lvalue into a wrapper constructor that has both a copy and a move overload, selecting the copy. They now std::move the local. Also: - vector::insert_back(T) / inserting_back(T) now push_back(std::move(value)) instead of copying the by-value parameter a second time. - set::keys() returns the local directly instead of std::move(vec), which was a pessimizing move that defeated NRVO. Docs: the operator[] / map const operator[] comments still claimed bounds checking was "enabled for debug builds". After the precondition changes the checks are always on (abort in debug and release); updated the comments and mention FCPP_NO_PRECONDITION_CHECKS.
Introduce a `counted` test type that tracks copy- vs move-constructions, and add tests proving the optimization rather than just behavior: - VectorTest.NonMutatingReturnMovesInsteadOfCopies (removing_back) - VectorTest.InsertBackMovesElementInsteadOfCopyingTwice - SetTest.NonMutatingReturnMovesInsteadOfCopies (removing) Each was verified to fail on the pre-fix code (extra container/element copies) and pass after, under both C++11 and C++17.
jkalias
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses #30 (performance). Eliminates unnecessary container copies in non-mutating operations, fixes an
insert_backdouble-copy and a pessimizing move, and refreshes inline docs that became stale after the precondition change in #29. Pure optimization — no behavior change for valid usage.1. Copy → move on non-mutating returns
Many non-mutating operations built a local
std::vector/std::set/std::map, then passed it as an lvalue into a wrapper constructor that has both a copy and a move overload — so the lvalue selected the copy, duplicating the whole container needlessly. They nowstd::movethe local into the wrapper:vector:filtered/filtered_parallel,sorted/sorted_parallel,removing_at/removing_back/removing_range,inserting_at/inserting_back,replacing_range_at,map/map_parallelset:difference_with/union_with/intersect_with,map,filtered,removing/inserting,zip_implmap:removing/inserting/filtered2.
insert_backdouble-copyvector::insert_back(T)/inserting_back(T)nowpush_back(std::move(value))instead of copying the by-value sink parameter a second time.3. Pessimizing move
set::keys()nowreturn vec;instead ofreturn std::move(vec);(the explicit move defeated NRVO;-Wpessimizing-move).4. Docs refresh
After #29 made precondition checks always-on, four
operator[]comments (vector + set) plusmap's constoperator[]still said "Bounds checking (assert) is enabled for debug builds." Updated them to state the checks run in both debug and release (std::abort) and to mention theFCPP_NO_PRECONDITION_CHECKSopt-out.Tests
Added a
countedtest type that tracks copy- vs move-constructions, and tests that prove the optimization rather than just behavior:VectorTest.NonMutatingReturnMovesInsteadOfCopies—removing_back: 7 → 4 copiesSetTest.NonMutatingReturnMovesInsteadOfCopies—removing: 6 → 3 copiesVectorTest.InsertBackMovesElementInsteadOfCopyingTwice— 2 copies/0 moves → 1 copy / 1 moveEach was verified to fail on the pre-fix code (with exactly the higher counts above) and pass after, under both C++11 and C++17.
Verification
Built and ran the full suite under Debug & Release × C++11 & C++17 — all green (305 tests C++17, 289 C++11).
Closes #30.
https://claude.ai/code/session_011y3XcVg3UCcgi5JMEo2tso