Add C++17 STL container converters #202
Conversation
Add converters for modern STL containers: - std::unordered_map<K,V> -> Python dict - std::unordered_set<T> -> Python set - std::deque<T> -> Python list - std::list<T> -> Python list - std::optional<T> -> Python T | None - std::string_view -> Python bytes/str Update default C++ standard from C++11 to C++17 for compilation. Fix converter architecture to handle None values for std::optional. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
WalkthroughAdds C++17 STL container support and converters (unordered_map, unordered_set, deque, list, optional, string_view); registers converters; updates codegen imports and default C++ standard to C++17; fixes std::optional None handling; and adds tests and C++ test headers exercising the new mappings. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
|
about the string_view I was not 100% sure |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tests/test_code_generator_new_stl.py (1)
82-137: Consider adding test for empty containers and additional edge cases.The runtime tests are thorough. A few optional additions for robustness:
- Empty container tests (empty dict, empty set, empty list)
- Empty string for
string_viewThese are nice-to-haves and the current coverage is adequate for the initial implementation.
tests/test_files/new_stl_test.hpp (1)
6-14: Remove unused<vector>include.The
<vector>header is included but not used in this file.#include <unordered_map> #include <unordered_set> #include <deque> #include <list> #include <optional> #include <string_view> #include <string> -#include <vector>autowrap/ConversionProvider.py (1)
2408-2435: Cython does support automatic conversion for std::unordered_set with primitive types. The web search confirms this capability exists in Cython's standard library.The original review comment is valid:
StdSetConverteruses Cython's automatic conversion (cdef libcpp_set[$inner] $temp_var = $argument_var), whileStdUnorderedSetConverteruses explicit manual iteration. Since Cython handles both containers the same way, the explicit iteration approach in lines 2408-2435 is unnecessarily verbose and potentially less efficient than leveraging Cython's automatic conversion.This is a legitimate optional refactoring opportunity to align
StdUnorderedSetConverterwith the simpler pattern used inStdSetConverter.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
CHANGELOG.md(1 hunks)autowrap/CodeGenerator.py(1 hunks)autowrap/ConversionProvider.py(2 hunks)autowrap/Utils.py(1 hunks)tests/test_code_generator_new_stl.py(1 hunks)tests/test_files/new_stl_test.hpp(1 hunks)tests/test_files/new_stl_test.pxd(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/test_code_generator_new_stl.py (2)
autowrap/__init__.py (1)
parse_and_generate_code(92-106)autowrap/Utils.py (1)
compile_and_import(65-147)
autowrap/ConversionProvider.py (2)
autowrap/Types.py (1)
CppType(42-289)autowrap/Code.py (3)
Code(41-90)add(56-75)render(88-90)
🪛 Clang (14.0.6)
tests/test_files/new_stl_test.hpp
[error] 6-6: 'unordered_map' file not found
(clang-diagnostic-error)
🪛 Ruff (0.14.6)
autowrap/ConversionProvider.py
2163-2163: Unused method argument: cpp_type
(ARG002)
2166-2166: Unused method argument: cpp_type
(ARG002)
2259-2259: Unused method argument: res_type
(ARG002)
2259-2259: Unused method argument: with_const
(ARG002)
2309-2309: Unused method argument: cpp_type
(ARG002)
2312-2312: Unused method argument: cpp_type
(ARG002)
2437-2437: Unused method argument: res_type
(ARG002)
2437-2437: Unused method argument: with_const
(ARG002)
2517-2517: Unused method argument: cpp_type
(ARG002)
2520-2520: Unused method argument: cpp_type
(ARG002)
2612-2612: Unused method argument: res_type
(ARG002)
2612-2612: Unused method argument: with_const
(ARG002)
2681-2681: Unused method argument: cpp_type
(ARG002)
2684-2684: Unused method argument: cpp_type
(ARG002)
2782-2782: Unused method argument: res_type
(ARG002)
2782-2782: Unused method argument: with_const
(ARG002)
2865-2865: Unused method argument: cpp_type
(ARG002)
2868-2868: Unused method argument: cpp_type
(ARG002)
2913-2913: Unused method argument: res_type
(ARG002)
2913-2913: Unused method argument: with_const
(ARG002)
2976-2976: Unused method argument: cpp_type
(ARG002)
2979-2979: Unused method argument: cpp_type
(ARG002)
2982-2982: Unused method argument: cpp_type
(ARG002)
3002-3002: Unused method argument: res_type
(ARG002)
3002-3002: Unused method argument: with_const
(ARG002)
3006-3006: Unused method argument: cpp_type
(ARG002)
⏰ 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 (>0.29.21, 3.10)
- GitHub Check: test (==3.1.0, 3.13)
- GitHub Check: test (==3.1.0, 3.10)
- GitHub Check: test (==3.1.0, 3.11)
- GitHub Check: test (==3.1.0, 3.12)
🔇 Additional comments (10)
autowrap/CodeGenerator.py (1)
2027-2036: LGTM! New STL imports and operator aliases are correctly added.The new libcpp imports follow the existing aliasing pattern and the additional operator imports (
preincrement as inc,address as address) are required by the new converter iteration code in ConversionProvider.py.CHANGELOG.md (1)
1-18: LGTM! Changelog entry is comprehensive and well-structured.The entry clearly documents all new STL container support, the C++ standard upgrade requirement, and the None handling fix. Good practice to note the breaking change (C++17 requirement) prominently.
tests/test_code_generator_new_stl.py (1)
16-77: LGTM! Comprehensive test coverage for new STL converters.The test covers:
- Code generation verification (import statements, method signatures, has_value pattern)
- Runtime behavior for all 6 new container types
- Edge cases like
Noneforstd::optionalThe test structure follows existing patterns in the codebase.
tests/test_files/new_stl_test.pxd (1)
1-56: LGTM! Well-documented PXD example for new STL containers.The file serves as both a test fixture and documentation showing users how to declare C++17 STL types in their PXD files. The comments explaining each container type's Python mapping are helpful.
tests/test_files/new_stl_test.hpp (1)
15-113: LGTM! Clean C++17 test implementation.The implementation is straightforward and correctly exercises all the new STL container types. The static analysis error about missing headers is a false positive from the sandbox environment lacking C++ standard library headers.
autowrap/ConversionProvider.py (4)
2140-2164: LGTM! StdUnorderedMapConverter follows established patterns.The converter correctly mirrors the existing
StdMapConverterimplementation while being appropriately simplified for the initial release. The static analysis warnings about unusedcpp_typearguments inmatches()andmatching_python_type()are false positives—these match the requiredTypeConverterBaseinterface.
2835-2942: LGTM! StdOptionalConverter correctly handles None values.The design is thoughtful:
- Uses
objectas Python type to allowNoneto pass throughtype_check_expressionvalidates the value is eitherNoneor the correct inner type- Input conversion correctly handles both
None(empty optional) and values- Output conversion properly checks
has_value()before accessing
2985-3009: Verify string_view lifetime safety in input conversion.The input conversion creates a Python
bytesobject that thestring_viewreferences. This is safe as long as the bytes object outlives the C++ function call, which it should since it's a local variable in the generated wrapper.However, the docstring (lines 2954-2959) correctly notes this lifetime concern. For safety, ensure users understand that
string_viewshould only be used for input parameters, not stored in C++ objects.
3112-3118: LGTM! Converters registered in correct order.The new converters are registered after the existing ones, which is the correct pattern. The comment accurately identifies the C++ standard versions.
autowrap/Utils.py (1)
92-100: C++17 flag missing for Windows, but Windows support not currently tested.The review comment correctly identifies that Windows lacks an explicit C++17 flag despite the codebase using C++17 features (
std::optional,std::string_viewin tests/test_files/new_stl_test.hpp). However, the CI pipeline only tests on Linux (ubuntu-latest), suggesting Windows support is not currently a priority.If Windows support is intended in the future, adding
/std:c++17to the MSVC compile flags would be appropriate since MSVC doesn't enable all C++17 features by default in all versions. For now, this appears to be a valid forward-looking concern rather than a blocking issue.
|
It looks like |
|
@copilot try to add and test the cleanup code. if not remove the deque support |
|
@timosachsenberg I've opened a new pull request, #207, to work on those changes. Once the pull request is ready, I'll request review from you. |
Since cython 3.2 also works, I'd be amenable to just dropping 3.0 support and testing on 3.1 and 3.2. |
|
Good idea! |
…#207) * Initial plan * Add tests for std::deque and std::list mutable reference cleanup code Co-authored-by: timosachsenberg <[email protected]> * Run code review and codeql checks Co-authored-by: timosachsenberg <[email protected]> * Remove CodeQL artifact and update gitignore Co-authored-by: timosachsenberg <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: timosachsenberg <[email protected]> Co-authored-by: Timo Sachsenberg <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/test_code_generator_new_stl.py (1)
147-153: Consider adding explicitstrcases forstring_viewcoverageRight now the
string_viewruntime checks only exercisebytesinputs. If you intend to support bothbytesandstron the Python side, consider adding analogous assertions using plainstrarguments togetStringViewLength/stringViewToStringso regressions there are caught.tests/test_files/new_stl_test.pxd (1)
19-58: PXD declarations match header semantics; consider const‑ref if you don’t want in/out behaviorThe
_NewSTLTestdeclarations correctly mirror the C++ header in terms of container types and return values. Using non‑const references for thesum*methods (libcpp_*&) is ABI‑safe against the currentconst &definitions and pairs with the wrappers’ “copy back” behavior to allow future mutating implementations.If you’re certain these methods should remain read‑only, you could tighten them to
const libcpp_*&to document that intent more clearly, but it isn’t required for correctness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.gitignore(1 hunks)tests/test_code_generator_new_stl.py(1 hunks)tests/test_files/inherited.pyx(2 hunks)tests/test_files/new_stl_test.hpp(1 hunks)tests/test_files/new_stl_test.pxd(1 hunks)tests/test_files/new_stl_test.pyx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_code_generator_new_stl.py (2)
autowrap/__init__.py (1)
parse_and_generate_code(92-106)autowrap/Utils.py (1)
compile_and_import(65-147)
🪛 Clang (14.0.6)
tests/test_files/new_stl_test.hpp
[error] 6-6: 'unordered_map' file not found
(clang-diagnostic-error)
🪛 Ruff (0.14.6)
tests/test_code_generator_new_stl.py
130-130: Loop control variable i not used within loop body
(B007)
⏰ 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). (4)
- GitHub Check: test (==3.2.0, 3.13)
- GitHub Check: test (==3.2.0, 3.12)
- GitHub Check: test (==3.1.0, 3.10)
- GitHub Check: test (==3.1.0, 3.12)
🔇 Additional comments (5)
.gitignore (1)
179-179: Verify the purpose and necessity of the CodeQL artifact entry.The entry
_codeql_detected_source_rootis added without a clarifying comment. While gitignore entries don't strictly require documentation, this artifact name is not immediately recognizable as a standard build or code-generation artifact.Please confirm:
- Is
_codeql_detected_source_roota documented artifact generated by the autowrap code generation pipeline (e.g., as part of the new C++17 STL support tooling)?- Should this entry have an associated comment for maintainability (e.g.,
# CodeQL artifactsor# Build-generated files)?- If this is unrelated to PR#202 objectives, consider separating it into a dedicated commit.
tests/test_files/inherited.pyx (1)
1-1: New STL libcpp cimports look consistent with converter additionsThe bumped generator header and added
libcpp_*cimports for unordered_map/unordered_set/deque/list/optional/string_view are consistent with the new C++17 container support. Having them present in this generated file even if unused here is fine.Also applies to: 14-19
tests/test_code_generator_new_stl.py (1)
16-82: End‑to‑end generation + runtime test is well structuredThis test exercises both code generation (imports, wrappers present) and runtime behavior through
compile_and_import, which gives good coverage for the new STL converters.tests/test_files/new_stl_test.hpp (1)
6-126: C++ test fixture cleanly exercises all new STL container shapesThe
NewSTLTestclass methods cover read‑only and mutating scenarios for unordered_map/set, deque, list, optional, and string_view, matching the Python wrappers and tests. Signatures (by‑value vsconst &vs mutable&) are well chosen for the behaviors you test.tests/test_files/new_stl_test.pyx (1)
49-188: Container converters (unordered_map/set, deque, list, optional) look correct and memory‑safeThe wrappers for unordered_map/unordered_set, deque, list, and optional correctly:
- Validate Python argument types.
- Build corresponding C++ containers (using stack objects or
new+delwhere appropriate).- Call into the
NewSTLTestmethods.- Propagate potential C++‑side mutations back into the Python dict/set/list via in‑place updates.
- Map
std::optional<int>toint | Nonecleanly, including theNone‑to‑empty‑optional case via default construction andvalue_or(-1).I don’t see correctness or lifetime issues in these sections.
|
@timosachsenberg I've opened a new pull request, #210, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: timosachsenberg <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
CHANGELOG.md (2)
4-12: Verify that all documented STL converters are fully implemented and tested.The new converters section documents six new C++17 STL types, but several PR discussion items suggest unresolved concerns that should be reflected or verified:
- std::deque support: PR comments indicate deque cleanup code needs to be added and tested, or the feature should be removed if cleanup cannot be implemented. Confirm this requirement has been addressed.
- std::string_view support: PR comments note that string_view support in Cython became available in version 3.1 and suggest dropping Cython 3.0 support in favor of 3.1+. Verify this constraint is reflected in CI/test configuration and documented if there are version-specific limitations.
Consider adding clarifying notes if any converters have platform, version, or implementation constraints (e.g., "std::string_view requires Cython ≥ 3.1").
13-18: Ensure C++ standard change and CI/tests are aligned.The CHANGELOG documents updating the default C++ standard from C++11 to C++17, which enables optional and string_view support. Per PR discussions, CI and tests should also be updated to target Cython ≥ 3.1 (dropping 3.0 support). Verify that:
- CI configuration has been updated to test against Cython 3.1 and 3.2 (as suggested in PR comments)
- Build/compilation documentation reflects the new C++17 requirement
- Any breaking changes for users still on older Cython versions are documented
If CI/test updates are not yet complete, I can help verify the changes or generate relevant configuration updates.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CHANGELOG.md(1 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). (8)
- GitHub Check: test (==3.2.0, 3.10)
- GitHub Check: test (==3.1.0, 3.11)
- GitHub Check: test (==3.2.0, 3.13)
- GitHub Check: test (==3.1.0, 3.12)
- GitHub Check: test (==3.2.0, 3.12)
- GitHub Check: test (==3.2.0, 3.11)
- GitHub Check: test (==3.1.0, 3.10)
- GitHub Check: test (==3.1.0, 3.13)
🔇 Additional comments (1)
CHANGELOG.md (1)
21-22: Clarify status of "arbitrary key types" support.The enriched summary indicates that a previous bullet about arbitrary key types support was removed, but lines 21–22 still document this feature in the final CHANGELOG. Please clarify:
- Is this feature included in the current PR, or should it be removed from this CHANGELOG entry?
- If retained, does it relate to or depend on the C++17/STL converter changes?
Fix unreachable str handling in string_view wrapper methods
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/test_files/new_stl_test.pyx (2)
113-187: Deque/list wrappers correctly mirror Python list semanticsUsing stack-allocated
libcpp_deque[int]/libcpp_list[double]and then re-slicing the original Python lists (d[:]/l[:]) cleanly exposes the C++ operations while avoiding any manual cleanup issues, and the O(n) list rebuilds are fine for test code. If you ever want to reduce boilerplate, these loops could be replaced by direct STL↔Python coercions, but that’s purely an optional refactor and not required for correctness here.
189-212: Optional[int] ↔ None mapping is implemented correctly; bool typing check is redundant
getOptionalValuecorrectly maps an engagedlibcpp_optional[int]to anintand a disengaged one toNone, andunwrapOptionalbuilds alibcpp_optional[int]only whenopt is not None, so the Optional[int]/None semantics look right. The combination ofbool hasValueplusassert isinstance(hasValue, pybool_t)is effectively redundant because Cython will already coerce truthy Python values to C++bool; it’s harmless, but you could simplify to an untyped Python parameter with an internalcdef boolif you want a more idiomatic interface.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_files/new_stl_test.pyx(1 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). (8)
- GitHub Check: test (==3.1.0, 3.13)
- GitHub Check: test (==3.1.0, 3.12)
- GitHub Check: test (==3.1.0, 3.10)
- GitHub Check: test (==3.2.0, 3.11)
- GitHub Check: test (==3.2.0, 3.12)
- GitHub Check: test (==3.2.0, 3.13)
- GitHub Check: test (==3.1.0, 3.11)
- GitHub Check: test (==3.2.0, 3.10)
🔇 Additional comments (2)
tests/test_files/new_stl_test.pyx (2)
49-111: STL unordered_map/unordered_set conversions are correct and memory-safeThe dict/set round-trips via
libcpp_unordered_map/libcpp_unordered_setlook good: keys/values are type-checked, C++ containers are built explicitly, mutations are propagated back to the Python inputs, andnewallocations are paired withdelso there’s no leak. This relies on Cython’s standard STL↔Python conversions and thec_string_encoding=asciidirective for string keys, which matches the intended bytes-based mapping here.
214-240: string_view wrappers now accept both bytes and str as intended and depend on Cython’s string_view supportChanging
getStringViewLength/stringViewToStringto take an untypedsvand handlingbytes/strviaisinstanceand UTF‑8 encoding fixes the earlier issue wherestrhandling was unreachable behind abytes-typed parameter and aligns with theUnion[bytes, str]docstrings. Casting the normalisedbytesbuffer tolibcpp_string_viewfor the C++ call is sound as long as the C++ side doesn’t store the view, and it matches Cython’sstd::string_viewfrom‑Py conversions that landed in the 3.1+ series; just ensure CI is actually running against the agreed Cython ≥3.1 versions.
|
The tests do not cover containers of wrapped classes or if you want to support it, nested containers. |
good point. I also wasn't sure about #210 which I closed. So in general we should increase coverage of wrapped classes and ideally nested container (but this was the stuff with recursively parsing template parameter?) |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
autowrap/ConversionProvider.py (2)
2750-2765: Reference cleanup gap for wrapper classes acknowledged.The past review comment correctly identified that wrapper class references in deque don't have cleanup code to reflect C++ modifications back to Python. This is acceptable if by-ref wrapper class deques are not a priority use case for this initial implementation.
2918-2933: Same reference cleanup gap as StdDequeConverter.Already noted in past review comments.
🧹 Nitpick comments (4)
autowrap/ConversionProvider.py (1)
2265-2268: Minor: Usenot infor membership test.Python style prefers
not inovernot ... infor readability.if ( tt_key.base_type in self.converters.names_of_wrapper_classes - and not tt_value.base_type in self.converters.names_of_wrapper_classes + and tt_value.base_type not in self.converters.names_of_wrapper_classes ):Same applies to line 2291.
tests/test_files/wrapped_container_test.hpp (3)
16-37: Consider consistency between comparison operators and marking single-arg constructor explicit.Two observations:
operator<compares onlyvalue_, whileoperator==compares bothvalue_andname_. This inconsistency could lead to confusing behavior where two Items are considered equivalent by ordered containers (std::set,std::map) but not equal byoperator==.The single-argument constructor
Item(int v)allows implicit conversions frominttoItem, which may be unintended.For a test file this may be acceptable, but for robustness:
- Item(int v) : value_(v), name_("item") {} + explicit Item(int v) : value_(v), name_("item") {}And consider aligning
operator<withoperator==:bool operator<(const Item& other) const { - return value_ < other.value_; + if (value_ != other.value_) return value_ < other.value_; + return name_ < other.name_; }
39-47: Consider includingname_in the hash for better distribution.The hash only uses
value_, whileoperator==compares bothvalue_andname_. This is functionally correct but may cause unnecessary hash collisions when items differ only by name.struct hash<Item> { size_t operator()(const Item& item) const { - return std::hash<int>()(item.value_); + size_t h1 = std::hash<int>()(item.value_); + size_t h2 = std::hash<std::string>()(item.name_); + return h1 ^ (h2 << 1); } };
58-64: Consider usingconstreferences for read-only container parameters.Multiple
sum*methods take non-const references but only read from the containers. Usingconst&would be more idiomatic and allow passing temporary containers.This applies to:
sumVectorItems,sumSetItems,sumMapValues,sumMapKeys,sumNestedVector,sumMapItemToVecInt,sumMapBoth,sumUnorderedMapKeys,sumUnorderedMapValues,sumUnorderedMapBoth,sumUnorderedMapItemToVecInt,sumListItems,sumDequeItems,sumUnorderedSetItems,sumListOfVectors,sumDequeOfVectors.Example fix for this method:
- int sumVectorItems(std::vector<Item>& items) { + int sumVectorItems(const std::vector<Item>& items) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.gitignore(1 hunks)autowrap/ConversionProvider.py(3 hunks)autowrap/PXDParser.py(1 hunks)tests/test_files/wrapped_container_test.hpp(1 hunks)tests/test_files/wrapped_container_test.pxd(1 hunks)tests/test_wrapped_containers.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
autowrap/PXDParser.py (1)
autowrap/Types.py (1)
CppType(42-289)
autowrap/ConversionProvider.py (2)
autowrap/Code.py (3)
Code(41-90)add(56-75)render(88-90)autowrap/Types.py (1)
CppType(42-289)
🪛 Clang (14.0.6)
tests/test_files/wrapped_container_test.hpp
[error] 4-4: 'vector' file not found
(clang-diagnostic-error)
🪛 Ruff (0.14.6)
autowrap/PXDParser.py
284-284: NameNode may be undefined, or defined from star imports
(F405)
287-287: IndexNode may be undefined, or defined from star imports
(F405)
autowrap/ConversionProvider.py
2184-2184: Unused method argument: cpp_type
(ARG002)
2187-2187: Unused method argument: cpp_type
(ARG002)
2267-2267: Test for membership should be not in
Convert to not in
(E713)
2291-2291: Test for membership should be not in
Convert to not in
(E713)
2359-2359: Unused method argument: res_type
(ARG002)
2359-2359: Unused method argument: with_const
(ARG002)
2483-2483: Unused method argument: cpp_type
(ARG002)
2486-2486: Unused method argument: cpp_type
(ARG002)
2612-2612: Unused method argument: res_type
(ARG002)
2612-2612: Unused method argument: with_const
(ARG002)
2692-2692: Unused method argument: cpp_type
(ARG002)
2695-2695: Unused method argument: cpp_type
(ARG002)
2787-2787: Unused method argument: res_type
(ARG002)
2787-2787: Unused method argument: with_const
(ARG002)
2856-2856: Unused method argument: cpp_type
(ARG002)
2859-2859: Unused method argument: cpp_type
(ARG002)
2957-2957: Unused method argument: res_type
(ARG002)
2957-2957: Unused method argument: with_const
(ARG002)
3040-3040: Unused method argument: cpp_type
(ARG002)
3043-3043: Unused method argument: cpp_type
(ARG002)
3088-3088: Unused method argument: res_type
(ARG002)
3088-3088: Unused method argument: with_const
(ARG002)
3151-3151: Unused method argument: cpp_type
(ARG002)
3154-3154: Unused method argument: cpp_type
(ARG002)
3157-3157: Unused method argument: cpp_type
(ARG002)
3177-3177: Unused method argument: res_type
(ARG002)
3177-3177: Unused method argument: with_const
(ARG002)
3181-3181: Unused method argument: cpp_type
(ARG002)
⏰ 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). (8)
- GitHub Check: test (==3.2.0, 3.12)
- GitHub Check: test (==3.2.0, 3.11)
- GitHub Check: test (==3.1.0, 3.13)
- GitHub Check: test (==3.2.0, 3.10)
- GitHub Check: test (==3.2.0, 3.13)
- GitHub Check: test (==3.1.0, 3.12)
- GitHub Check: test (==3.1.0, 3.10)
- GitHub Check: test (==3.1.0, 3.11)
🔇 Additional comments (11)
.gitignore (1)
175-180: LGTM!The additions correctly ignore the generated wrapper file for the new wrapped container tests and the CodeQL artifact.
autowrap/PXDParser.py (1)
282-293: LGTM - Improved nested template handling.The expanded loop correctly handles the three cases for template arguments:
NameNode: Simple type names (e.g.,Iteminmap[int, Item])IndexNode: Nested templates (e.g.,vector[int]inmap[int, vector[int]])CComplexBaseTypeNode: Types with declarators (pointers, references)The static analysis warnings about
NameNodeandIndexNodebeing undefined are false positives—they're imported via the star import fromCython.Compiler.ExprNodeson line 45.autowrap/ConversionProvider.py (4)
1106-1127: LGTM - Proper handling of maps with both wrapped key and value.The new output conversion correctly wraps both keys and values when they are wrapped classes, creating new Python wrapper objects with proper
shared_ptrmanagement.
3010-3058: LGTM - Well-designed Optional handling.The design correctly uses
objectas the Python type to allowNonewhile validating the actual type at runtime. The docstring clearly documents this intentional choice.
3180-3184: Verifystring_viewoutput safety in C++ implementations.The output conversion correctly creates a Python bytes copy from the
string_view. However, if C++ returns astring_viewpointing to stack-allocated or freed memory, this will crash or produce garbage. This is inherent tostring_viewsemantics, but consider adding a note in documentation that C++ functions returningstring_viewmust ensure the underlying data remains valid.
3287-3293: LGTM - Correct converter registration.All six new STL converters are properly registered after the existing converters and before class-specific converters, matching the established pattern.
tests/test_wrapped_containers.py (2)
1-42: LGTM - Comprehensive test fixture and setup.The module-scoped fixture correctly compiles the test wrapper, and the naming convention (
wrapped_container_wrapper.pyx) avoids Cython redeclaration issues as documented in the comment.
45-509: LGTM - Thorough container test coverage.Excellent coverage of:
- All container types (vector, set, map, list, deque, unordered variants)
- Wrapped classes as keys, values, and both
- Nested containers (vector<vector>, list<vector>)
- Edge cases (empty containers)
- Item class construction, methods, and copy semantics
The tests validate both input conversion (Python → C++) and output conversion (C++ → Python).
tests/test_files/wrapped_container_test.pxd (1)
1-130: LGTM - Cython declarations properly paired with C++ header.The verification confirms
wrapped_container_test.hppexists attests/test_files/wrapped_container_test.hpp, matching the extern declaration on line 13 of the pxd file. The declarations are well-structured with comprehensive coverage of container scenarios and clear sectional comments documenting test coverage.tests/test_files/wrapped_container_test.hpp (2)
1-14: LGTM - Well-structured header with appropriate includes.The include guard follows standard conventions, and all necessary STL headers are included for the container types used throughout the file.
Note: The static analysis error about
'vector' file not foundis a false positive - this is a standard C++ header that requires a properly configured C++ toolchain.
50-355: Well-structured test class with comprehensive container coverage.The
WrappedContainerTestclass provides excellent coverage of STL container types with wrapped class elements:
- Ordered containers:
vector,set,map- Unordered containers:
unordered_map,unordered_set- Sequential containers:
list,deque- Nested containers:
vector<vector<Item>>,list<vector<int>>,deque<vector<int>>- Wrapped class as key, value, or both
The clear section headers and consistent naming conventions (
sum*,create*,appendTo*) make the test scenarios easy to follow.
|
@jpfeuffer added |
|
Yes nested containers will trigger the recursive part of the if cases in our conversion providers. We only support it for vectors currently. |
|
@timosachsenberg I think this still does not test hashing or any kind of lookups for dicts. |
Add converters for modern STL containers:
Update default C++ standard from C++11 to C++17 for compilation. Fix converter architecture to handle None values for std::optional.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.