Add lookup/membership tests for STL containers and fix variable name collision bug #216
Conversation
…collision bug - Add tests for hash-based lookups in unordered_map (find, count, at) - Add tests for membership checks in unordered_set (count, find) - Add lookup tests for wrapped class containers (map, unordered_map, unordered_set) - Fix bug where loop variables 'key'/'value' could shadow function parameters of the same name in StdMapConverter and StdUnorderedMapConverter The new tests verify that dict/set operations actually use hashing for O(1) lookups rather than just iterating through all items.
WalkthroughThis PR refactors STL container iteration variable naming in the code generator's conversion logic to use mangled names ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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: 1
🧹 Nitpick comments (1)
tests/test_files/new_stl_test.pyx (1)
346-372: Minor docstring/assertion inconsistency in string_view methods.The docstrings declare parameter type as
bytes, but the assertions check for(bytes, str). While functionally harmless (the assertion is more permissive), this creates a documentation inconsistency.Either update the docstring to match the assertion:
- getStringViewLength(self, sv: bytes ) -> int + getStringViewLength(self, sv: Union[bytes, str] ) -> intOr update the assertion to match the docstring:
- assert isinstance(sv, (bytes, str)), 'arg sv wrong type' + assert isinstance(sv, bytes), 'arg sv wrong type'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
autowrap/ConversionProvider.py(3 hunks)tests/test_code_generator_new_stl.py(3 hunks)tests/test_files/new_stl_test.hpp(2 hunks)tests/test_files/new_stl_test.pxd(1 hunks)tests/test_files/new_stl_test.pyx(6 hunks)tests/test_files/wrapped_container_test.hpp(3 hunks)tests/test_files/wrapped_container_test.pxd(3 hunks)tests/test_wrapped_containers.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
tests/test_wrapped_containers.py (2)
tests/test_files/new_stl_test.hpp (9)
m(23-29)m(31-37)m(31-31)m(40-46)m(40-40)m(49-51)m(49-49)m(54-56)m(54-54)tests/test_files/wrapped_container_test.hpp (27)
m(102-108)m(102-102)m(119-125)m(119-119)m(128-130)m(128-128)m(136-142)Item(21-21)Item(22-22)Item(23-23)Item(24-24)items(58-64)items(58-58)items(74-76)items(74-74)items(82-88)items(82-82)items(300-306)items(300-300)items(320-326)items(320-320)items(340-346)items(340-340)items(357-359)items(357-357)items(362-368)items(362-362)
autowrap/ConversionProvider.py (1)
autowrap/Code.py (1)
add(56-75)
tests/test_files/new_stl_test.hpp (1)
tests/test_files/wrapped_container_test.hpp (16)
m(102-108)m(102-102)m(119-125)m(119-119)m(128-130)m(128-128)m(136-142)m(136-136)m(176-185)m(176-176)m(191-197)m(191-191)m(211-217)m(211-211)m(231-237)m(231-231)
🪛 Ruff (0.14.6)
tests/test_code_generator_new_stl.py
126-126: Do not assert False (python -O removes these calls), raise AssertionError()
Replace assert False
(B011)
127-128: try-except-pass detected, consider logging the exception
(S110)
127-127: Do not catch blind exception: Exception
(BLE001)
⏰ 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.13)
- GitHub Check: test (==3.2.0, 3.12)
- GitHub Check: test (==3.1.0, 3.10)
- 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.1.0, 3.11)
- GitHub Check: test (==3.1.0, 3.12)
🔇 Additional comments (21)
autowrap/ConversionProvider.py (3)
860-863: Excellent bug fix for variable name collision.The introduction of mangled loop variable names (
loop_keyandloop_value) properly addresses the issue where loop variables could shadow function parameters with the same names. Usingmangle()with theargument_varensures unique variable names in the generated Cython code.
865-972: Consistent application of mangled variables throughout the method.The mangled
loop_keyandloop_valuevariables are correctly used in all conversion expressions and the loop iteration (line 972). This ensures the fix is complete and prevents collisions across all code paths in the StdMapConverter.
2237-2260: Bug fix correctly applied to StdUnorderedMapConverter.The same mangled variable approach is consistently implemented here, mirroring the fix in StdMapConverter. This ensures both map-based converters are protected from parameter name collision issues.
tests/test_files/new_stl_test.hpp (2)
39-56: LGTM! Clean implementation of unordered_map accessor methods.The three accessor methods follow established patterns:
find()for safe lookup with sentinel return,count()for existence check, andat()for throwing accessor. This aligns well with the patterns inwrapped_container_test.hpp(lookupMapIntToItem,hasKeyMapIntToItem).
74-91: LGTM! Consistent unordered_set accessor implementations.The set methods mirror the map patterns appropriately. While
findUnorderedSetreturning the found value seems redundant (you already know what you're searching for), it effectively validates that the hash-basedfind()operation works correctly through the Python bindings.tests/test_files/new_stl_test.pxd (1)
37-46: LGTM! Correct Cython declarations with proper exception handling.The
except +annotation ongetValueUnorderedMapis correctly applied sincestd::unordered_map::at()throwsstd::out_of_rangefor missing keys. The other methods use safe patterns (returning sentinel values) and don't require exception handling.tests/test_files/wrapped_container_test.hpp (3)
118-130: LGTM! Proper map lookup implementations.The
find()-based lookup andcount()-based existence check follow the established patterns. Usingconstreference for the map parameter is correct.
247-259: LGTM! Consistent unordered_map implementations.These mirror the
std::mapversions exactly, which is appropriate since both containers share the same lookup interface.
356-368: LGTM! Clean unordered_set membership and find implementations.The hash-based membership test and find operation correctly use the Item's hash function defined earlier in the file.
tests/test_code_generator_new_stl.py (2)
47-62: LGTM! Good coverage of generated method assertions.The assertions verify that all new lookup and membership methods are correctly generated in the output.
138-152: LGTM! Comprehensive unordered_set membership and find tests.Good coverage of both present and missing values, verifying the hash-based O(1) operations work correctly through the bindings.
tests/test_wrapped_containers.py (3)
147-178: LGTM! Thorough map lookup and key existence tests.Good coverage testing both existing keys (1, 5, 10) and missing keys (999, 3). The tests properly verify the -1 sentinel return for missing keys.
341-372: LGTM! Consistent unordered_map tests matching the map tests.The docstrings correctly note "hash-based O(1) lookup", which aligns with the PR objective of verifying that dict/set operations use hashing rather than iteration.
503-541: LGTM! Well-designed unordered_set membership and find tests.Creating new
Iteminstances (search_item1,search_item2) for lookup rather than reusing the original references is a good pattern - it verifies that the hash function and equality operator work correctly for Items with the same value.tests/test_files/wrapped_container_test.pxd (3)
54-55: LGTM! Correct Cython declarations for map lookups.
94-95: LGTM! Correct Cython declarations for unordered_map lookups.
125-126: LGTM! Correct Cython declarations for unordered_set operations.tests/test_files/new_stl_test.pyx (4)
26-26: LGTM! Added shared_ptr import for proper memory management.
67-68: Good fix: Loop variables renamed to prevent parameter shadowing.The change from generic
key/valueloop variables to_loop_key_m/_loop_value_mprevents the bug where function parameters namedkeyorvaluewould be shadowed by the loop variables. This aligns with the PR objective of fixing the variable name collision bug in StdMapConverter and StdUnorderedMapConverter.
81-145: LGTM! Correctly generated unordered_map wrapper methods.The three new methods (
lookupUnorderedMap,hasKeyUnorderedMap,getValueUnorderedMap) follow the established pattern for container conversion and sync-back. The_loop_key_m/_loop_value_mnaming is consistently applied.
179-243: LGTM! Correctly generated unordered_set wrapper methods.The set methods properly handle the conversion to C++
libcpp_unordered_setand sync modifications back to the Python set.
- Update Item hash function to combine both value_ and name_ members - Add test verifying that items with same value_ but different name_ are treated as distinct (tests proper multi-field hashing)
| key_conv_cleanup = "" | ||
|
|
||
| # Use mangled variable names to avoid collision with function parameters | ||
| loop_key = mangle("_loop_key_" + argument_var) |
There was a problem hiding this comment.
@jpfeuffer is this extra mangling safer? needed?
| return py_result | ||
|
|
||
| def getStringViewLength(self, sv ): | ||
| def getStringViewLength(self, bytes sv ): |
There was a problem hiding this comment.
@jpfeufer no clue here. are both needed or now only one sufficient?
- Rename test files: new_stl_test.* -> cpp17_stl_test.* - Rename test module: test_code_generator_new_stl.py -> test_code_generator_cpp17_stl.py - Rename class: NewSTLTest -> Cpp17STLTest - Add user-centric documentation with container mappings and usage examples - Document Python types for each C++ container (dict, set, list, T|None, bytes) - Include example syntax for passing/receiving containers - Document compilation requirements (C++17)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
tests/test_code_generator_cpp17_stl.py (4)
143-143: Remove extraneous f-prefix from string literal.This f-string has no placeholders.
- f"lookupUnorderedMap('grape') should return -1 for missing key" + "lookupUnorderedMap('grape') should return -1 for missing key"
157-161: Improve exception handling: usepytest.raisesor catch specific exception.Using bare
except Exceptionwithpassobscures which exception is expected and silences unintended errors. Additionally,assert Falseis removed bypython -O.- # Verify at() throws exception for missing key - try: - obj.getValueUnorderedMap(test_map, b"missing") - assert False, "getValueUnorderedMap should raise exception for missing key" - except Exception: - pass # Expected - std::out_of_range from at() + # Verify at() throws exception for missing key + with pytest.raises(Exception): # std::out_of_range from at() + obj.getValueUnorderedMap(test_map, b"missing")
196-196: Remove extraneous f-prefix from string literal.This f-string has no placeholders.
- f"findUnorderedSet(999) should return -1 for missing element" + "findUnorderedSet(999) should return -1 for missing element"
238-240: Unused loop variableiin assertion.The loop variable
iis declared but never used in the loop body. Consider using_or incorporatingiinto the error message.- for i, (r, e) in enumerate(zip(list_data, expected_doubled)): - assert abs(r - e) < 0.0001, \ - f"doubleListElements should modify list in place: {list_data}" + for idx, (r, e) in enumerate(zip(list_data, expected_doubled)): + assert abs(r - e) < 0.0001, \ + f"doubleListElements mismatch at index {idx}: {list_data}"tests/test_files/cpp17_stl_test.pyx (2)
346-358: Docstring mismatch with assertion forgetStringViewLength.The docstring on line 348 says
sv: bytes, but the assertion on line 350 accepts bothbytesandstr. This is intentional (allowing str with UTF-8 encoding), but the docstring should reflect the actual accepted types.def getStringViewLength(self, bytes sv ): """ - getStringViewLength(self, sv: bytes ) -> int + getStringViewLength(self, sv: Union[bytes, str] ) -> int """
360-372: Same docstring inconsistency forstringViewToString.The docstring says
bytesbut the implementation accepts bothbytesandstr.def stringViewToString(self, bytes sv ): """ - stringViewToString(self, sv: bytes ) -> bytes + stringViewToString(self, sv: Union[bytes, str] ) -> bytes """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
tests/test_code_generator_cpp17_stl.py(1 hunks)tests/test_code_generator_new_stl.py(0 hunks)tests/test_files/cpp17_stl_test.hpp(1 hunks)tests/test_files/cpp17_stl_test.pxd(1 hunks)tests/test_files/cpp17_stl_test.pyx(7 hunks)tests/test_files/new_stl_test.pxd(0 hunks)
💤 Files with no reviewable changes (2)
- tests/test_code_generator_new_stl.py
- tests/test_files/new_stl_test.pxd
🧰 Additional context used
🪛 Clang (14.0.6)
tests/test_files/cpp17_stl_test.hpp
[error] 19-19: 'unordered_map' file not found
(clang-diagnostic-error)
🪛 Ruff (0.14.6)
tests/test_code_generator_cpp17_stl.py
143-143: f-string without any placeholders
Remove extraneous f prefix
(F541)
159-159: Do not assert False (python -O removes these calls), raise AssertionError()
Replace assert False
(B011)
160-161: try-except-pass detected, consider logging the exception
(S110)
160-160: Do not catch blind exception: Exception
(BLE001)
196-196: f-string without any placeholders
Remove extraneous f prefix
(F541)
238-238: 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). (7)
- GitHub Check: test (==3.2.0, 3.10)
- GitHub Check: test (==3.2.0, 3.13)
- GitHub Check: test (==3.2.0, 3.11)
- GitHub Check: test (==3.1.0, 3.10)
- GitHub Check: test (==3.2.0, 3.12)
- GitHub Check: test (==3.1.0, 3.11)
- GitHub Check: test (==3.1.0, 3.12)
🔇 Additional comments (17)
tests/test_code_generator_cpp17_stl.py (4)
1-44: LGTM! Comprehensive module docstring and proper setup.The docstring clearly documents container mappings, usage examples, and compilation requirements. Imports are appropriate for the test context.
68-104: Good coverage of code generation verification.The assertions verify all expected imports and method signatures are present in the generated code. This provides good regression protection for the code generator.
119-276: Comprehensive runtime test coverage for all C++17 STL containers.The test thoroughly exercises:
unordered_map: return, iteration, find, count, atunordered_set: return, iteration, membership, count, finddeque: return, iteration, mutable referencelist: return, iteration, mutable referenceoptional: value/None handlingstring_view: length and round-tripEdge cases like missing keys/elements are properly covered.
212-216: Mutable reference behavior is correctly implemented.The binding properly handles in-place modification through the standard Cython pattern:
- Creates a C++ deque from the Python list
- Calls the C++ method to modify it
- Copies the modified deque back to the original Python list using slice assignment (
d[:] = [...]on line 275 of the .pyx file)The test at lines 212-216 will pass as expected because the binding correctly reflects modifications back to the Python list.
tests/test_files/cpp17_stl_test.pyx (3)
67-68: Good fix: Loop variables renamed to avoid shadowing function parameters.The renaming from
key/valueto_loop_key_m/_loop_value_mprevents variable name collisions when the function has parameters namedkeyorvalue. This addresses the bug fix mentioned in the PR objectives.
81-101: New lookup method correctly implements hash-based key lookup.The
lookupUnorderedMapmethod properly:
- Validates input types
- Converts Python dict to C++ unordered_map
- Calls the C++ lookup with the key parameter
- Copies any modifications back to the Python dict
- Returns the result
The loop variable naming (
_loop_key_m,_loop_value_m) correctly avoids shadowing thekeyparameter.
179-199: Implementation pattern is consistent across new unordered_set methods.The
hasValueUnorderedSetmethod follows the same pattern as the map methods, with proper type validation, C++ container conversion, method call, and write-back. The loop variable naming avoids shadowing thevalueparameter.tests/test_files/cpp17_stl_test.pxd (3)
1-46: Excellent documentation in the PXD file.The header comments clearly document container type mappings, required imports, and compilation requirements. This serves as good reference documentation for users of autowrap.
65-65: Correct use ofexcept +for exception-throwing method.The
getValueUnorderedMapmethod correctly declaresexcept +to propagate the C++std::out_of_rangeexception fromat()to Python. This enables the test's exception checking to work correctly.
47-127: Complete and consistent method declarations.All methods in the C++ header are properly declared with matching signatures. The use of references (
&) for container parameters is correct for the mutable reference semantics.tests/test_files/cpp17_stl_test.hpp (7)
19-27: Standard C++ includes are correct.The clang error about
'unordered_map' file not foundis a false positive from the static analysis environment, which likely lacks proper C++ standard library paths or C++17 support configured. The includes are standard C++17 headers.
28-30: Class structure is clean and appropriate for testing.The default constructor is sufficient for this test class that has no member state.
56-74: Hash-based lookup methods correctly demonstrate O(1) operations.The
lookupUnorderedMap,hasKeyUnorderedMap, andgetValueUnorderedMapmethods properly usefind(),count(), andat()respectively—all O(1) average-time operations for hash-based containers. This aligns with the PR's goal of testing hash-based lookups.
106-114:findUnorderedSetreturns the searched value itself when found.This is an unusual pattern—typically
findeither returns an iterator or a boolean. Returning the value itself (which the caller already has) isn't particularly useful. Consider whether this is the intended behavior or if it should return a different indicator.The current implementation returns
valuewhen found, which the caller already knows. This seems designed purely to test thatfind()works, which is valid for a test case, but the API design is unconventional.
136-142: Mutable reference correctly allows in-place modification.The
doubleDequeElementsmethod takes a non-const reference, enabling modifications to be reflected back to the caller. The Cython wrapper correctly copies the modified container back to the Python list.
177-189: Clean std::optional implementation using value_or.The
getOptionalValueandunwrapOptionalmethods cleanly demonstrate optional value handling. Usingvalue_or(-1)is idiomatic C++17.
197-205: string_view handling is correct.The methods properly accept
std::string_viewand demonstrate conversion tostd::string. Note that the Python side passes bytes, and the Cython layer handles the conversion correctly.
|
ok I was able to add an example with hashing. I see some looping but I am not sure if / where it could be prevented. |
|
ok I merge and try to build openms with it |
|
This is not really what I meant. Now it implemented functions on the cython level to do lookups. I just want to see that d[key] == value in python. With a wrapped class. And I have no idea why this name mangling should be needed. Was there an error somewhere? |
of the same name in StdMapConverter and StdUnorderedMapConverter
The new tests verify that dict/set operations actually use hashing for O(1)
lookups rather than just iterating through all items.
Summary by CodeRabbit
New Features
Tests
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.