Skip to content

Add std::move for container by-value parameters #215

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
timosachsenberg wants to merge 1 commit into
base: master
Choose a base branch
from
+95 −16
Open

Add std::move for container by-value parameters #215

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions autowrap/CodeGenerator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2041,6 +2041,7 @@ def create_default_cimports(self):
code.add(
"""
|from libcpp.memory cimport shared_ptr
|from libcpp.utility cimport move
"""
)
if self.include_numpy:
Expand Down
110 changes: 94 additions & 16 deletions autowrap/ConversionProvider.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1082,7 +1082,12 @@ def input_conversion(
else:
cleanup_code = "del %s" % temp_var

return code, "deref(%s)" % temp_var, cleanup_code
# Use move() for by-value parameters to avoid copying large maps
if cpp_type.is_ref:
call_as = "deref(%s)" % temp_var
else:
call_as = "move(deref(%s))" % temp_var
return code, call_as, cleanup_code

def call_method(self, res_type: CppType, cy_call_str: str, with_const: bool = True) -> str:
return "_r = %s" % cy_call_str
Expand Down Expand Up @@ -1251,7 +1256,12 @@ def input_conversion(
)
else:
cleanup_code = "del %s" % temp_var
return code, "deref(%s)" % temp_var, cleanup_code
# 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
Comment on lines +1259 to +1264
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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_var

Also 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.

Copy link
Collaborator Author

@timosachsenberg timosachsenberg Nov 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jpfeuffer ?

Copy link
Contributor

@coderabbitai coderabbitai bot Nov 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!


elif tt.base_type in self.converters.names_of_wrapper_classes:
base_type = tt.base_type
Expand Down Expand Up @@ -1293,7 +1303,12 @@ def input_conversion(

else:
cleanup_code = "del %s" % temp_var
return code, "deref(%s)" % temp_var, cleanup_code
# Use move() for by-value parameters to avoid copying large sets
if cpp_type.is_ref:
call_as = "deref(%s)" % temp_var
else:
call_as = "move(deref(%s))" % temp_var
return code, call_as, cleanup_code
else:
inner = self.converters.cython_type(tt)
# cython cares for conversion of stl containers with std types:
Expand Down Expand Up @@ -1726,7 +1741,12 @@ def input_conversion(
)
else:
cleanup_code = "del %s" % temp_var
return code, "deref(%s)" % temp_var, cleanup_code
# Use move() for by-value parameters to avoid copying large vectors
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

elif tt.base_type in self.converters.names_of_wrapper_classes:
# Case 2: We wrap a std::vector<> with a base type we need to wrap
Expand All @@ -1752,8 +1772,11 @@ def input_conversion(

if cpp_type.is_ptr:
call_fragment = temp_var
else:
elif cpp_type.topmost_is_ref:
call_fragment = "deref(%s)" % temp_var
else:
# Use move() for by-value parameters to avoid copying large vectors
call_fragment = "move(deref(%s))" % temp_var

return code, call_fragment, cleanup_code

Expand Down Expand Up @@ -1802,7 +1825,12 @@ def input_conversion(
locals(),
)

return code, "%s" % temp_var, cleanup_code
# Use move() for by-value parameters to avoid copying large vectors
if cpp_type.topmost_is_ref:
call_as = "%s" % temp_var
else:
call_as = "move(%s)" % temp_var
return code, call_as, cleanup_code

elif inner_contains_classes_to_wrap and tt.base_type != "libcpp_vector":
# Only if the template argument which is neither a class-to-wrap nor a std::vector
Expand Down Expand Up @@ -1857,7 +1885,12 @@ def input_conversion(
"Error: For recursion in std::vector<T> to work, we need a ConverterRegistry instance at self.cr"
)

return code, "deref(%s)" % temp_var, cleanup_code
# Use move() for by-value parameters to avoid copying large vectors
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

else:
# Case 5: We wrap a regular type
Expand All @@ -1879,7 +1912,12 @@ def input_conversion(
locals(),
)

return code, "%s" % temp_var, cleanup_code
# Use move() for by-value parameters to avoid copying large vectors
if cpp_type.topmost_is_ref:
call_as = "%s" % temp_var
else:
call_as = "move(%s)" % temp_var
return code, call_as, cleanup_code

def call_method(self, res_type: CppType, cy_call_str: str, with_const: bool = True) -> str:
t = self.converters.cython_type(res_type)
Expand Down Expand Up @@ -2354,7 +2392,12 @@ def input_conversion(
else:
cleanup_code = "del %s" % temp_var

return code, "deref(%s)" % temp_var, cleanup_code
# Use move() for by-value parameters to avoid copying large maps
if cpp_type.is_ref:
call_as = "deref(%s)" % temp_var
else:
call_as = "move(deref(%s))" % temp_var
return code, call_as, cleanup_code

def call_method(self, res_type: CppType, cy_call_str: str, with_const: bool = True) -> str:
return "_r = %s" % cy_call_str
Expand Down Expand Up @@ -2543,7 +2586,12 @@ def input_conversion(
)
else:
cleanup_code = "del %s" % temp_var
return code, "deref(%s)" % temp_var, cleanup_code
# 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
Comment on lines +2589 to +2594
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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_var

Also 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.


elif tt.base_type in self.converters.names_of_wrapper_classes:
base_type = tt.base_type
Expand Down Expand Up @@ -2579,7 +2627,12 @@ def input_conversion(
)
else:
cleanup_code = "del %s" % temp_var
return code, "deref(%s)" % temp_var, cleanup_code
# Use move() for by-value parameters to avoid copying large sets
if cpp_type.is_ref:
call_as = "deref(%s)" % temp_var
else:
call_as = "move(deref(%s))" % temp_var
return code, call_as, cleanup_code
else:
# Primitive types - need explicit iteration
item = "item%d" % arg_num
Expand Down Expand Up @@ -2607,7 +2660,12 @@ def input_conversion(
)
else:
cleanup_code = "del %s" % temp_var
return code, "deref(%s)" % temp_var, cleanup_code
# Use move() for by-value parameters to avoid copying large sets
if cpp_type.is_ref:
call_as = "deref(%s)" % temp_var
else:
call_as = "move(deref(%s))" % temp_var
return code, call_as, cleanup_code

def call_method(self, res_type: CppType, cy_call_str: str, with_const: bool = True) -> str:
return "_r = %s" % cy_call_str
Expand Down Expand Up @@ -2745,7 +2803,12 @@ def input_conversion(
)
else:
cleanup_code = "del %s" % temp_var
return code, "deref(%s)" % temp_var, cleanup_code
# 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
Comment on lines +2806 to +2811
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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_var

Also 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.


elif tt.base_type in self.converters.names_of_wrapper_classes:
base_type = tt.base_type
Expand All @@ -2762,7 +2825,12 @@ def input_conversion(
locals(),
)
cleanup_code = "del %s" % temp_var
return code, "deref(%s)" % temp_var, cleanup_code
# Use move() for by-value parameters to avoid copying large deques
if cpp_type.is_ref:
call_as = "deref(%s)" % temp_var
else:
call_as = "move(deref(%s))" % temp_var
return code, call_as, cleanup_code
else:
# Primitive types - need explicit iteration
item = "item%d" % arg_num
Expand Down Expand Up @@ -2913,7 +2981,12 @@ def input_conversion(
)
else:
cleanup_code = "del %s" % temp_var
return code, "deref(%s)" % temp_var, cleanup_code
# 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
Comment on lines +2984 to +2989
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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_var

Also 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.


elif tt.base_type in self.converters.names_of_wrapper_classes:
base_type = tt.base_type
Expand All @@ -2930,7 +3003,12 @@ def input_conversion(
locals(),
)
cleanup_code = "del %s" % temp_var
return code, "deref(%s)" % temp_var, cleanup_code
# Use move() for by-value parameters to avoid copying large lists
if cpp_type.is_ref:
call_as = "deref(%s)" % temp_var
else:
call_as = "move(deref(%s))" % temp_var
return code, call_as, cleanup_code
else:
code = Code().add(
"""
Expand Down
Loading