Conversation
|
Warning Rate limit exceeded@isVoid has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 4 minutes and 23 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a new qual_name field to many AST types, initializes it from Clang's qualified-name APIs with fallbacks, exposes it in pybind11 (including updated pickle/unpickle shapes), propagates it into Python wrappers, and adds tests validating qualified-name behavior across scopes. Changes
Sequence Diagram(s)sequenceDiagram
participant Clang as Clang AST
participant Cpp as C++ objects
participant Bind as pybind11
participant Py as Python wrappers
participant Pickle as Pickle/Unpickle
Clang->>Cpp: getQualifiedNameAsString()
Cpp->>Cpp: initialize qual_name (fallback to name if empty)
Note over Cpp: Enum, Function, Record, Typedef,\nFunctionTemplate, ClassTemplate, ConstExprVar
Cpp->>Bind: expose qual_name via def_readwrite
Bind->>Py: from_c_obj reads c_obj.qual_name and stores it
Py->>Pickle: include qual_name in pickle tuple
Pickle->>Py: unpickle passes qual_name back to constructor
rect rgb(240,248,255)
Note over Bind,Py: qual_name is propagated end-to-end and validated by tests
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ast_canopy/cpp/src/detail/matchers/record_matcher.cpp (1)
47-59: Fix variable name typo:rename_for_unamed→rename_for_unnamed.The variable is misspelled consistently across lines 53, 57, and 59. While this doesn't affect runtime behavior, it reduces code clarity and consistency.
🔎 Proposed fix
- std::string rename_for_unamed = + std::string rename_for_unnamed = - name.empty() ? "unnamed" + std::to_string(id) : name; + name.empty() ? "unnamed" + std::to_string(id) : name; auto &record_id_map = *payload->record_id_to_name; - record_id_map[id] = rename_for_unamed; + record_id_map[id] = rename_for_unnamed; payload->decls->records.push_back(Record( - RD, RecordAncestor::ANCESTOR_IS_NOT_TEMPLATE, rename_for_unamed)); + RD, RecordAncestor::ANCESTOR_IS_NOT_TEMPLATE, rename_for_unnamed));
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (14)
ast_canopy/ast_canopy/decl.pyast_canopy/ast_canopy/pylibastcanopy.cppast_canopy/ast_canopy/pylibastcanopy.pyiast_canopy/cpp/include/ast_canopy/ast_canopy.hppast_canopy/cpp/src/class_template.cppast_canopy/cpp/src/constexpr_vardecl.cppast_canopy/cpp/src/detail/matchers/record_matcher.cppast_canopy/cpp/src/enum.cppast_canopy/cpp/src/function.cppast_canopy/cpp/src/function_template.cppast_canopy/cpp/src/record.cppast_canopy/cpp/src/typedef.cppast_canopy/tests/data/sample_qual_name.cuast_canopy/tests/test_qual_name.py
🧰 Additional context used
🧬 Code graph analysis (5)
ast_canopy/tests/test_qual_name.py (2)
ast_canopy/ast_canopy/api.py (1)
parse_declarations_from_source(336-494)ast_canopy/ast_canopy/decl.py (3)
qual_name(423-424)name(419-420)name(565-566)
ast_canopy/ast_canopy/decl.py (4)
ast_canopy/ast_canopy/pylibastcanopy.pyi (5)
Record(77-87)name(135-135)name(154-154)name(176-176)name(193-193)ast_canopy/cpp/include/ast_canopy/ast_canopy.hpp (4)
Record(186-196)Record(197-207)Record(208-208)Record(209-210)ast_canopy/cpp/src/record.cpp (2)
Record(23-98)Record(100-107)numbast/src/numbast/functor.py (1)
name(55-58)
ast_canopy/ast_canopy/pylibastcanopy.pyi (3)
ast_canopy/ast_canopy/decl.py (4)
qual_name(423-424)FunctionTemplate(198-233)Template(175-195)Function(62-172)ast_canopy/cpp/include/ast_canopy/ast_canopy.hpp (15)
ParamVar(96-96)ParamVar(97-97)Type(44-44)Type(45-46)Type(47-47)FunctionTemplate(151-155)FunctionTemplate(156-160)FunctionTemplate(161-161)Template(104-107)Template(108-108)Template(112-112)Template(112-112)Function(128-132)Function(133-137)Function(138-138)ast_canopy/cpp/src/function.cpp (1)
Function(32-66)
ast_canopy/cpp/src/enum.cpp (2)
ast_canopy/ast_canopy/decl.py (3)
qual_name(423-424)name(419-420)name(565-566)ast_canopy/ast_canopy/pylibastcanopy.pyi (4)
name(135-135)name(154-154)name(176-176)name(193-193)
ast_canopy/cpp/src/class_template.cpp (1)
ast_canopy/ast_canopy/decl.py (1)
qual_name(423-424)
🔇 Additional comments (33)
ast_canopy/cpp/src/detail/matchers/record_matcher.cpp (1)
47-51: Well-documented placeholder naming strategy.The comment clearly explains how anonymous records are handled, the placeholder naming convention, and the stability caveat. The implementation correctly assigns stable-within-a-run identifiers to anonymous records using Clang's internal Decl ID.
ast_canopy/cpp/src/record.cpp (2)
32-37: LGTM: Robust fallback for anonymous records.The qualified name initialization with fallback to the unqualified name ensures stable behavior for anonymous records where Clang may return an empty qualified name.
104-106: LGTM: Consistent fallback logic.The fallback logic in this constructor overload matches the primary constructor, maintaining consistency across both initialization paths.
ast_canopy/cpp/src/typedef.cpp (1)
19-22: LGTM: Consistent qualified name handling.The initialization pattern matches other entity types (Record, ConstExprVar) with appropriate fallback for empty qualified names.
ast_canopy/cpp/src/class_template.cpp (1)
17-22: LGTM: Appropriate fallback strategy for class templates.The fallback to
record.qual_nameis appropriate since the class template's qualified name should align with its templated record. This relies on the Record constructor's guarantee thatrecord.qual_nameis never empty (it falls back toname).ast_canopy/cpp/src/constexpr_vardecl.cpp (1)
12-16: LGTM: Consistent qualified name initialization.The initialization follows the established pattern with appropriate fallback for empty qualified names.
ast_canopy/cpp/include/ast_canopy/ast_canopy.hpp (7)
62-69: LGTM: Enum qualified name integration.The constructor signature and field addition properly integrate qual_name into the Enum structure.
81-81: LGTM: ConstExprVar qualified name field.The qual_name field addition is consistent with other entity types.
131-141: LGTM: Function qualified name constructors.The two-constructor pattern provides both a simple default (qual_name = name) and an explicit qual_name parameter, enabling flexible initialization paths.
155-162: LGTM: FunctionTemplate qualified name handling.The constructors appropriately initialize qual_name from the function's qual_name by default, with an overload for explicit qualification.
193-213: LGTM: Record qualified name constructors.Both constructor overloads properly handle qual_name initialization, with the first defaulting to name and the second accepting an explicit parameter.
228-228: LGTM: ClassTemplate qualified name field.The field addition aligns with the pattern across other entity types.
234-242: LGTM: Typedef qualified name constructors.The constructor pattern matches Record's approach with sensible defaults and explicit parameter support.
ast_canopy/cpp/src/function_template.cpp (1)
20-24: LGTM with a note: Fallback depends on function.qual_name.The fallback to
function.qual_nameis appropriate for function templates. However, this assumes thatfunction.qual_nameis never empty. Given the observation infunction.cppthat the Function constructor lacks fallback logic, this could potentially propagate empty qual_name values.Verify that the Function constructor (in function.cpp) properly handles empty qualified names to ensure this fallback is reliable.
ast_canopy/tests/data/sample_qual_name.cu (1)
1-75: LGTM: Excellent test fixture with comprehensive coverage.This test fixture provides well-documented examples covering various qualified name scenarios:
- Global scope entities
- Anonymous C-style typedef records
- Anonymous namespaces (with
(anonymous namespace)marker)- Nested namespaces
The inline comments serve as valuable executable documentation for the expected qual_name behavior.
ast_canopy/cpp/src/enum.cpp (1)
10-16: LGTM! Consistent qual_name initialization.The qualified name initialization follows the same pattern used across other entity types in this PR (Record, Function, etc.), with appropriate fallback to
namewhen the qualified name is empty. This ensures stable behavior for anonymous or unnamed enums.ast_canopy/ast_canopy/pylibastcanopy.pyi (1)
8-8: LGTM! Type stub declarations are correct.The
qual_name: strfield has been consistently added to all relevant entity types (ClassTemplate, ConstExprVar, Enum, Function, FunctionTemplate, Record, Typedef), matching the C++ bindings exposed inpylibastcanopy.cpp.Also applies to: 19-19, 39-39, 54-54, 61-61, 82-82, 118-118
ast_canopy/ast_canopy/decl.py (8)
69-82: LGTM! Function qual_name integration is correct.The
qual_namefield is properly initialized in the constructor and correctly extracted from the C++ binding object infrom_c_obj.Also applies to: 161-172
205-229: LGTM! FunctionTemplate qual_name integration is correct.The
qual_namefield is properly initialized and extracted from the C++ binding, maintaining consistency with the Function class.
243-299: LGTM! StructMethod qual_name integration is correct.The
qual_nameparameter is properly threaded through the constructor and correctly passed to the parentFunctionclass, maintaining the inheritance chain.
339-416: LGTM! Struct qual_name integration is correct.The
qual_namefield follows the established pattern: stored in_qual_nameand exposed via a property, consistent with thenamefield. Thefrom_c_objmethod correctly extracts it from the C++ binding.
422-424: LGTM! Property exposure is consistent.The
qual_nameproperty follows the same pattern as thenameproperty, maintaining consistency within the class.
435-454: LGTM! TemplatedStruct qual_name integration is correct.The
from_c_objmethod correctly passesc_obj.qual_nameto the parentStructconstructor.
464-486: LGTM! ClassTemplate qual_name integration is correct.The
qual_namefield is properly initialized and extracted from the C++ binding, maintaining consistency across template types.
522-540: LGTM! ClassTemplateSpecialization correctly propagates qual_name.The constructor correctly passes
record.qual_nameto the parentStructconstructor, ensuring qualified name information flows through specializations.ast_canopy/tests/test_qual_name.py (1)
1-182: Excellent test coverage!The test suite comprehensively validates the
qual_namefeature across:
- ✅ All entity types (structs, functions, templates, enums, typedefs)
- ✅ Named namespace scoping (
ns1::ns2::...)- ✅ Global scope (unqualified names)
- ✅ Edge case: anonymous records with C-style typedef linkage
- ✅ Edge case: anonymous namespace marker handling
The tests are well-documented, use appropriate assertions, and properly exercise both wrapped declarations (
decl.*) and raw bindings (pylibastcanopy.*).ast_canopy/ast_canopy/pylibastcanopy.cpp (7)
47-66: LGTM! Enum pickle/unpickle correctly updated.The serialization logic properly includes
qual_nameas the 4th element, the unpickle validation checks for 4 elements, and the constructor receives all fields in the correct order.
90-95: LGTM! ConstExprVar qual_name exposure is correct.The
qual_namefield is properly exposed viadef_readwrite.
154-176: LGTM! Function pickle/unpickle correctly updated.The serialization logic properly includes
qual_nameas the 5th element, the unpickle validation checks for 5 elements, and the constructor receives all fields in the correct order.
195-213: LGTM! FunctionTemplate pickle/unpickle correctly updated.The serialization logic properly includes
qual_nameas the 3rd element, the unpickle validation checks for 3 elements, and the constructor receives all fields including the qual_name fromt[2].
215-219: LGTM! ClassTemplate qual_name exposure is correct.The
qual_namefield is properly exposed viadef_readwrite.
238-268: LGTM! Record pickle/unpickle correctly updated.The serialization logic properly includes
qual_nameas the 10th element, the unpickle validation checks for 10 elements, and the constructor receives all fields with qual_name correctly positioned att[9].
270-284: LGTM! Typedef pickle/unpickle correctly updated.The serialization logic properly includes
qual_nameas the 3rd element, the unpickle validation checks for 3 elements, and the constructor receives all fields in the correct order.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ast_canopy/ast_canopy/pylibastcanopy.cpp (1)
221-236: Method unpickle losesqual_namefrom the base Function.When unpickling a Method, the code extracts the Function
f(which containsqual_name), but then constructs the Method using onlyf.name, f.return_type, f.params, f.exec_space— discardingf.qual_name. The Method constructor callsFunction(name, return_type, params, exec_space)which defaultsqual_nametoname, losing the original qualified name.🔎 Proposed fix to preserve qual_name during Method unpickle
This requires either:
- Adding a Method constructor that accepts qual_name, or
- Manually setting qual_name after construction:
[](py::tuple t) { if (t.size() != 2) throw std::runtime_error( "Invalid class template state during unpickle!"); Function f = t[0].cast<Function>(); - return Method{f.name, f.return_type, f.params, f.exec_space, - t[1].cast<method_kind>()}; + Method m{f.name, f.return_type, f.params, f.exec_space, + t[1].cast<method_kind>()}; + m.qual_name = f.qual_name; + return m; }));ast_canopy/cpp/include/ast_canopy/ast_canopy.hpp (1)
168-180: Consider adding a Method constructor that acceptsqual_name.The Method struct inherits from Function but its constructor only calls the 4-arg
Function(name, return_type, params, exec_space)constructor, which defaultsqual_nametoname. This means Method instances cannot have a distinctqual_nameset during construction, which affects pickle round-trip correctness (as noted in the bindings file).🔎 Proposed constructor addition
struct Method : public Function { Method(const std::string &name, const Type &return_type, const std::vector<ParamVar> ¶ms, const execution_space &exec_space, const method_kind &kind) : Function(name, return_type, params, exec_space), kind(kind) {} + Method(const std::string &name, const Type &return_type, + const std::vector<ParamVar> ¶ms, const execution_space &exec_space, + const method_kind &kind, const std::string &qual_name) + : Function(name, return_type, params, exec_space, qual_name), kind(kind) {} Method(const clang::CXXMethodDecl *); method_kind kind;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
ast_canopy/ast_canopy/pylibastcanopy.cppast_canopy/cpp/include/ast_canopy/ast_canopy.hpp
🔇 Additional comments (14)
ast_canopy/ast_canopy/pylibastcanopy.cpp (7)
69-88: LGTM!The Enum binding correctly exposes
qual_nameand the pickle/unpickle logic properly serializes all 5 fields in the correct order matching the constructor signature.
90-95: LGTM!The
qual_namefield is correctly exposed forConstExprVar.
154-176: LGTM!The Function binding correctly adds
qual_nameexposure and updates pickle/unpickle with consistent parameter ordering.
195-213: LGTM!The FunctionTemplate binding correctly exposes
qual_nameand the pickle/unpickle properly handles the inheritance from Template while adding the new qual_name field.
215-219: LGTM!The
qual_namefield is correctly exposed forClassTemplate.
238-268: LGTM!The Record binding correctly exposes
qual_nameand the pickle/unpickle properly handles all 10 fields in the correct order matching the constructor signature.
270-284: LGTM!The Typedef binding correctly exposes
qual_nameand the pickle/unpickle properly handles all 3 fields in the correct order.ast_canopy/cpp/include/ast_canopy/ast_canopy.hpp (7)
60-75: LGTM!The Enum struct correctly adds the
qual_namemember and provides a value-based constructor for unpickling while retaining the clang-based constructor.
77-85: LGTM!The
qual_namemember is correctly added toConstExprVar.
129-150: LGTM!The Function struct correctly adds the
qual_namemember with two constructor overloads: one defaultingqual_nametonamefor backward compatibility, and another accepting an explicitqual_namefor unpickling.
152-166: LGTM!The FunctionTemplate struct correctly adds
qual_namewith two constructor overloads: one derivingqual_namefromfunction.qual_name, and another accepting an explicitqual_namefor unpickling.
187-226: LGTM!The Record struct correctly adds
qual_namewith two constructor overloads: one defaultingqual_nametoname, and another accepting an explicitqual_namefor unpickling.
228-232: LGTM!The
qual_namemember is correctly added toClassTemplate.
234-246: LGTM!The Typedef struct correctly adds
qual_namewith two constructor overloads: one defaultingqual_nametoname, and another accepting an explicitqual_namefor unpickling.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
ast_canopy/ast_canopy/pylibastcanopy.cppast_canopy/cpp/include/ast_canopy/ast_canopy.hppast_canopy/cpp/src/detail/matchers/record_matcher.cpp
🧰 Additional context used
🧬 Code graph analysis (2)
ast_canopy/cpp/src/detail/matchers/record_matcher.cpp (2)
ast_canopy/cpp/include/ast_canopy/ast_canopy.hpp (4)
Record(189-199)Record(200-210)Record(211-211)Record(212-213)ast_canopy/cpp/src/record.cpp (2)
Record(23-98)Record(100-107)
ast_canopy/ast_canopy/pylibastcanopy.cpp (1)
ast_canopy/ast_canopy/decl.py (3)
name(416-417)name(562-563)qual_name(420-421)
🔇 Additional comments (16)
ast_canopy/ast_canopy/pylibastcanopy.cpp (8)
69-88: LGTM!The
qual_namefield has been properly added to theEnumbinding, and the pickle/unpickle logic correctly handles the new field. The tuple size check (5 elements) matches the actual tuple structure.
90-95: LGTM!The
qual_namefield has been properly added to theConstExprVarbinding.
154-176: LGTM!The
qual_namefield has been properly added to theFunctionbinding, and the pickle/unpickle logic correctly handles the new field. The tuple size check (5 elements) matches the actual tuple structure.
195-213: LGTM!The
qual_namefield has been properly added to theFunctionTemplatebinding, and the pickle/unpickle logic correctly handles the new field. The tuple size check (3 elements) matches the actual tuple structure.
215-219: LGTM!The
qual_namefield has been properly added to theClassTemplatebinding.
221-237: LGTM!The
Methodunpickle logic has been properly updated to passqual_namefrom theFunctionbase to theMethodconstructor.
239-269: LGTM!The
qual_namefield has been properly added to theRecordbinding, and the pickle/unpickle logic correctly handles the new field. The tuple size check (10 elements) matches the actual tuple structure.
271-285: LGTM!The
qual_namefield has been properly added to theTypedefbinding, and the pickle/unpickle logic correctly handles the new field. The tuple size check (3 elements) matches the actual tuple structure.ast_canopy/cpp/include/ast_canopy/ast_canopy.hpp (8)
60-75: LGTM!The
qual_namefield has been properly added to theEnumstruct with a new constructor overload that acceptsqual_nameas a parameter. The existing Clang-based constructor is preserved for backward compatibility.
77-85: LGTM!The
qual_namefield has been properly added to theConstExprVarstruct.
129-150: LGTM!The
qual_namefield has been properly added to theFunctionstruct. The existing constructor initializesqual_nametonameby default, and a new constructor overload allows explicit specification ofqual_name. The Clang-based constructor is preserved.
152-166: LGTM!The
qual_namefield has been properly added to theFunctionTemplatestruct. The existing constructor initializesqual_namefromfunction.qual_nameby default, and a new constructor overload allows explicit specification. The Clang-based constructor is preserved.
168-181: LGTM!The
Methodconstructor has been properly updated to accept aqual_nameparameter and pass it to theFunctionbase class constructor.
188-227: LGTM!The
qual_namefield has been properly added to theRecordstruct. The existing constructor initializesqual_nametonameby default, and a new constructor overload allows explicit specification ofqual_name. The Clang-based constructors are preserved.
229-233: LGTM!The
qual_namefield has been properly added to theClassTemplatestruct.
235-247: LGTM!The
qual_namefield has been properly added to theTypedefstruct. The existing constructor initializesqual_nametonameby default, and a new constructor overload allows explicit specification ofqual_name. The Clang-based constructor is preserved.
This PR is a follow up of #274, adds documentation for qualified name. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added comprehensive documentation for the qualified names attribute, detailing derivation details, edge case handling, and a complete reference of supported argument types with practical examples. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Michael Wang <isVoid@users.noreply.github.com>
1 participant
Summary by CodeRabbit
New Features
Behavior
Tests
✏️ Tip: You can customize this high-level summary in your review settings.