Add std::move for container by-value parameters #215
Conversation
Use move semantics when passing temporary containers (vector, map, set, deque, list) to C++ functions that take them by value. This avoids an unnecessary copy of the entire container after it has been populated. - Add `from libcpp.utility cimport move` to default imports - Modify all container converters to use `move(deref(temp))` or `move(temp)` when the C++ parameter is not a reference - Keep `deref(temp)` for reference parameters (no change in behavior) This optimization has the biggest impact for large containers passed to functions taking by-value parameters.
WalkthroughTwo files are modified to optimize parameter handling in generated code. An import of C++ move semantics is added to the code generator, and conversion logic is updated across multiple STL wrapper types to use move-based handling for by-value parameters, replacing direct dereference with move-wrapped dereference. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
autowrap/CodeGenerator.py(1 hunks)autowrap/ConversionProvider.py(16 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: test (==3.2.0, 3.13)
- GitHub Check: test (==3.1.0, 3.13)
- GitHub Check: test (==3.1.0, 3.11)
- GitHub Check: test (==3.1.0, 3.12)
- GitHub Check: test (==3.1.0, 3.10)
🔇 Additional comments (4)
autowrap/CodeGenerator.py (1)
2044-2044: LGTM! Move import enables optimization.The import of
movefromlibcpp.utilityis correctly placed and enables the use of move semantics in the generated Cython code for container parameters.autowrap/ConversionProvider.py (3)
1085-1090: LGTM! Move optimization correctly applied for maps.The logic correctly uses
move(deref(temp_var))for by-value parameters andderef(temp_var)for reference parameters. This avoids unnecessary copies when the C++ function takes the map by value.
1744-1749: LGTM! Vector move optimization correctly handles nested containers.The use of
topmost_is_refis appropriate forStdVectorConvertersince it explicitly supports nested vectors (e.g.,vector<vector<T>>). The move semantics are correctly applied across all cases (enum, wrapper classes, shared_ptr, nested vectors, and primitives).Also applies to: 1775-1779, 1828-1833, 1888-1893, 1915-1920
2395-2400: LGTM! Unordered map optimization correctly applied.The move optimization follows the same correct pattern as
StdMapConverter, usingis_refconsistently to distinguish by-value from by-reference parameters.
| # Use move() for by-value parameters to avoid copying large sets | ||
| if cpp_type.topmost_is_ref: | ||
| call_as = "deref(%s)" % temp_var | ||
| else: | ||
| call_as = "move(deref(%s))" % temp_var | ||
| return code, call_as, cleanup_code |
There was a problem hiding this comment.
Inconsistent use of topmost_is_ref vs is_ref.
Line 1260 uses cpp_type.topmost_is_ref for the enum case, while line 1307 uses cpp_type.is_ref for the wrapper class case. Both are checking whether to use move semantics for the same logical condition.
For consistency and correctness, consider using cpp_type.is_ref in both cases (matching the cleanup code checks at lines 1243 and 1286). While topmost_is_ref may be intentional for nested containers, StdSetConverter doesn't explicitly handle nesting like StdVectorConverter does.
Apply this diff to make the checks consistent:
- if cpp_type.topmost_is_ref:
+ if cpp_type.is_ref:
call_as = "deref(%s)" % temp_var
else:
call_as = "move(deref(%s))" % temp_varAlso applies to: 1306-1311
🤖 Prompt for AI Agents
In autowrap/ConversionProvider.py around lines 1259-1264 and 1306-1311, the code
inconsistently uses cpp_type.topmost_is_ref in the enum/stdset branch but
cpp_type.is_ref in the wrapper/class branch; update the enum/stdset branch to
use cpp_type.is_ref (matching the cleanup checks at lines 1243 and 1286) so both
branches use the same reference test, and ensure the logic and variable names
(call_as and cleanup_code) remain unchanged.
There was a problem hiding this comment.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| # Use move() for by-value parameters to avoid copying large sets | ||
| if cpp_type.topmost_is_ref: | ||
| call_as = "deref(%s)" % temp_var | ||
| else: | ||
| call_as = "move(deref(%s))" % temp_var | ||
| return code, call_as, cleanup_code |
There was a problem hiding this comment.
Inconsistent use of topmost_is_ref vs is_ref.
Line 2590 uses cpp_type.topmost_is_ref for the enum case, while lines 2631 and 2664 use cpp_type.is_ref for the wrapper class and primitive cases. This is the same inconsistency as in StdSetConverter.
For consistency with the cleanup code checks (lines 2573, 2610, 2647) and since StdUnorderedSetConverter doesn't explicitly handle nested containers, use cpp_type.is_ref uniformly.
Apply this diff to make the check consistent:
- if cpp_type.topmost_is_ref:
+ if cpp_type.is_ref:
call_as = "deref(%s)" % temp_var
else:
call_as = "move(deref(%s))" % temp_varAlso applies to: 2630-2635, 2663-2668
🤖 Prompt for AI Agents
In autowrap/ConversionProvider.py around lines 2589-2594 (and also apply the
same change at 2630-2635 and 2663-2668), the code checks cpp_type.topmost_is_ref
when deciding whether to call move(deref(...)) for by-value parameters; this is
inconsistent with nearby cleanup checks which use cpp_type.is_ref and with other
converters. Replace uses of cpp_type.topmost_is_ref with cpp_type.is_ref in
these locations so the reference check is uniform with the surrounding code and
cleanup logic.
| # Use move() for by-value parameters to avoid copying large deques | ||
| if cpp_type.topmost_is_ref: | ||
| call_as = "deref(%s)" % temp_var | ||
| else: | ||
| call_as = "move(deref(%s))" % temp_var | ||
| return code, call_as, cleanup_code |
There was a problem hiding this comment.
Inconsistent use of topmost_is_ref vs is_ref.
Line 2807 uses cpp_type.topmost_is_ref for the enum case, while line 2829 uses cpp_type.is_ref for the wrapper class case. Use cpp_type.is_ref consistently to match the cleanup code and since StdDequeConverter doesn't handle nesting.
Apply this diff:
- if cpp_type.topmost_is_ref:
+ if cpp_type.is_ref:
call_as = "deref(%s)" % temp_var
else:
call_as = "move(deref(%s))" % temp_varAlso applies to: 2828-2833
🤖 Prompt for AI Agents
In autowrap/ConversionProvider.py around lines 2806-2811 and 2828-2833, the code
inconsistently checks cpp_type.topmost_is_ref in the enum-handling branch but
uses cpp_type.is_ref in the wrapper-class branch; change the topmost_is_ref
checks to cpp_type.is_ref in both places so both branches use cpp_type.is_ref
consistently (since StdDequeConverter doesn't handle nesting) and keep the
cleanup/code paths aligned.
| # Use move() for by-value parameters to avoid copying large lists | ||
| if cpp_type.topmost_is_ref: | ||
| call_as = "deref(%s)" % temp_var | ||
| else: | ||
| call_as = "move(deref(%s))" % temp_var | ||
| return code, call_as, cleanup_code |
There was a problem hiding this comment.
Inconsistent use of topmost_is_ref vs is_ref.
Line 2985 uses cpp_type.topmost_is_ref for the enum case, while line 3007 uses cpp_type.is_ref for the wrapper class case. Use cpp_type.is_ref consistently to match the cleanup code and since StdListConverter doesn't handle nesting.
Apply this diff:
- if cpp_type.topmost_is_ref:
+ if cpp_type.is_ref:
call_as = "deref(%s)" % temp_var
else:
call_as = "move(deref(%s))" % temp_varAlso applies to: 3006-3011
🤖 Prompt for AI Agents
In autowrap/ConversionProvider.py around lines 2984-2989 and also 3006-3011, the
code uses cpp_type.topmost_is_ref in the enum handling but cpp_type.is_ref in
the wrapper class handling; update the enum branch to use cpp_type.is_ref (same
as the wrapper case) so the check is consistent with the cleanup logic and
StdListConverter's non-nested behavior, i.e., replace topmost_is_ref with is_ref
in both locations.
Use move semantics when passing temporary containers (vector, map, set,
deque, list) to C++ functions that take them by value. This avoids an
unnecessary copy of the entire container after it has been populated.
from libcpp.utility cimport moveto default importsmove(deref(temp))ormove(temp)when the C++ parameter is not a referencederef(temp)for reference parameters (no change in behavior)This optimization has the biggest impact for large containers passed
to functions taking by-value parameters.
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.