Fix crashes when parsing complex C++ template hierarchies#319
Open
Fix crashes when parsing complex C++ template hierarchies#319
Conversation
ast_canopy segfaulted or threw unhandled exceptions when parsing headers with deep template metaprogramming (CRTP hierarchies, dependent types, template template parameters, expression-based template arguments). Root causes and fixes: 1. function.cpp: mangleName() segfaults on dependent (uninstantiated) template types. Added has_dependent_signature() check to skip mangling for methods with dependent parameter/return types. Also fixed a memory leak (ItaniumMangleContext now uses unique_ptr). 2. type.cpp: getCanonicalType() crashes on dependent types. Added null- QualType guard and skip canonicalization for dependent types. 3. record.cpp: getTypeSize()/getTypeAlign() crash on dependent/incomplete types. Added isDependentType()/isIncompleteType() guards and try-catch around child declaration construction. 4. typedef.cpp: record_id_to_name->at() throws for template instantiations not in the map. Replaced with safe find() + fallback to Clang's name. Also added null guard for getAsCXXRecordDecl(). 5. template_param.cpp: TemplateTemplateParmDecl constructor threw. Now handles gracefully with kind=template_. 6. class_template_specialization.cpp: Unsupported template argument kinds (Pack, Template, Expression) threw. Now produces "<unsupported>" placeholder. 7. All matchers (7 files): Added try-catch around push_back calls so a single bad declaration is skipped instead of crashing the entire parse. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThe PR enhances error resilience and edge-case handling across the C++ AST parsing pipeline by adding exception-catching blocks to prevent processing failures, implementing defensive null/validity checks for dependent and incomplete types, and refactoring template parameter and type canonicalization logic to gracefully skip or fallback rather than throw on unsupported constructs. New test coverage validates parsing of complex template hierarchies. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ast_canopy/cpp/src/detail/matchers/class_template_matcher.cpp (1)
16-19:⚠️ Potential issue | 🟠 MajorBug: Constructor clears wrong collection.
ClassTemplateCallbackclearsfunction_templatesinstead ofclass_templates. This appears to be a copy-paste error and could cause function templates to be unexpectedly cleared when the class template matcher initializes.🐛 Proposed fix
ClassTemplateCallback::ClassTemplateCallback(traverse_ast_payload *payload) : payload(payload) { - payload->decls->function_templates.clear(); + payload->decls->class_templates.clear(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ast_canopy/cpp/src/detail/matchers/class_template_matcher.cpp` around lines 16 - 19, The constructor ClassTemplateCallback::ClassTemplateCallback(traverse_ast_payload *payload) mistakenly clears payload->decls->function_templates; change it to clear the class template collection instead by calling the correct member (payload->decls->class_templates) so initializing ClassTemplateCallback does not wipe function template data; locate the constructor in class_template_matcher.cpp and replace the function_templates clear with class_templates clear.ast_canopy/cpp/src/record.cpp (1)
19-21:⚠️ Potential issue | 🟡 MinorExpose
INVALID_SIZE_OF/INVALID_ALIGN_OFconstants to consumers.These sentinel values are defined locally in
record.cpp(lines 19-21) but not exposed in the header. Thesizeof_andalignof_members of theRecordstruct are public, and downstream consumers (Python bindings, tests) access them directly with no way to detect when they contain sentinel values. Tests assert specific values (e.g.,== 16) or check!= 0without handling the sentinel case.Consider exposing these constants in the header or documenting the sentinel value convention so consumers can properly validate results.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ast_canopy/cpp/src/record.cpp` around lines 19 - 21, Define and expose the sentinel constants INVALID_SIZE_OF and INVALID_ALIGN_OF in the public header where Record is declared so consumers can detect sentinel values; add constexpr declarations (matching the cpp definitions) in the same namespace as Record (so consumers and bindings can reference INVALID_SIZE_OF and INVALID_ALIGN_OF), ensure the header includes the required <limits> or <cstddef> headers, and update any tests/bindings to compare Record::sizeof_ and Record::alignof_ against these exposed constants rather than assuming specific numeric values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ast_canopy/cpp/src/detail/matchers/record_matcher.cpp`:
- Around line 56-64: The map entry for record_id_map
(payload->record_id_to_name) is added before constructing the Record, leaving
stale entries if Record(RD, ...) throws; move the insertion record_id_map[id] =
rename_for_unnamed so it happens only after the Record is successfully created
and pushed into payload->decls->records (i.e., place it after the push_back), or
alternatively, keep it before but in the catch block erase the key
(record_id_map.erase(id)) to ensure no lingering entry; reference record_id_map,
payload->record_id_to_name, Record(...), payload->decls->records,
rename_for_unnamed and RD when making the change.
In `@ast_canopy/cpp/src/detail/matchers/typedef_matcher.cpp`:
- Around line 41-46: The catch-all in typedef_matcher.cpp silently swallows
exceptions when pushing Typedef into payload->decls->typedefs; modify the
exception handling in the try/catch around Typedef(TD,
payload->record_id_to_name) to catch std::exception (or catch and rethrow after
logging) and log the typedef identifier/name (obtainable from TD or via
payload->record_id_to_name) on failure—at minimum emit a debug-level message
with the typedef name and exception.what(), so only non-fatal failures are
skipped but diagnostics are available for debugging.
In `@ast_canopy/cpp/src/function.cpp`:
- Around line 79-80: The dependent-signature fallback currently assigns
mangled_name = qual_name in has_dependent_signature(FD), collapsing all
dependent overloads; instead append or incorporate the spelled function type
(the parameter list) into mangled_name so overloads remain distinct (e.g., use
FD's spelled/printed function type or parameter-type-string and concatenate it
to qual_name), updating the branch where has_dependent_signature(FD) is handled
to produce qual_name + spelledFunctionType (or a deterministically formatted
parameter-list string) rather than just qual_name.
In `@ast_canopy/cpp/src/typedef.cpp`:
- Around line 31-38: The current fallback uses
underlying_record_decl->getNameAsString()/getQualifiedNameAsString(), which
drops template arguments for ClassTemplateSpecializationDecl; instead obtain the
QualType spelling and assign underlying_name from qd.getAsString() (the same
spelling used on line 43) so specializations like Storage<float, 3> keep their
template arguments; update the code around underlying_record_decl and
underlying_name to use the QualType (qd) spelling (qd.getAsString()) when
forming underlying_name, preserving template specialization arguments for
ClassTemplateSpecializationDecl/TemplateSpecializationType.
In `@ast_canopy/tests/test_complex_template_parsing.py`:
- Around line 95-96: The test docstring for
test_typedef_to_template_instantiation is inaccurate: it mentions a C++
using-declaration but the test uses a typedef form; update the docstring in
test_typedef_to_template_instantiation to reflect the actual sample (e.g.,
"Typedefs like 'typedef Storage<float, 3> Vec3fStorage;' should parse.") or
alternatively change the test input to use the using form so the docstring
matches; ensure the text references Vec3fStorage and Storage<float, 3> to keep
intent clear.
- Around line 41-85: The tests repeatedly call parse_declarations_from_source
causing redundant parsing; create a class-scoped pytest fixture (e.g., def
decls(self, source_path): return parse_declarations_from_source(source_path,
[source_path], "sm_80", bypass_parse_error=True)) and update each test method in
the class to accept that fixture (replace local calls to
parse_declarations_from_source with using the decls variable), keeping unique
assertions (use decls.class_templates and decls.functions) so parsing runs once
per test class.
---
Outside diff comments:
In `@ast_canopy/cpp/src/detail/matchers/class_template_matcher.cpp`:
- Around line 16-19: The constructor
ClassTemplateCallback::ClassTemplateCallback(traverse_ast_payload *payload)
mistakenly clears payload->decls->function_templates; change it to clear the
class template collection instead by calling the correct member
(payload->decls->class_templates) so initializing ClassTemplateCallback does not
wipe function template data; locate the constructor in
class_template_matcher.cpp and replace the function_templates clear with
class_templates clear.
In `@ast_canopy/cpp/src/record.cpp`:
- Around line 19-21: Define and expose the sentinel constants INVALID_SIZE_OF
and INVALID_ALIGN_OF in the public header where Record is declared so consumers
can detect sentinel values; add constexpr declarations (matching the cpp
definitions) in the same namespace as Record (so consumers and bindings can
reference INVALID_SIZE_OF and INVALID_ALIGN_OF), ensure the header includes the
required <limits> or <cstddef> headers, and update any tests/bindings to compare
Record::sizeof_ and Record::alignof_ against these exposed constants rather than
assuming specific numeric values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d72645da-9185-4797-8579-79005d09df3d
📒 Files selected for processing (16)
ast_canopy/cpp/src/class_template_specialization.cppast_canopy/cpp/src/detail/matchers/class_template_matcher.cppast_canopy/cpp/src/detail/matchers/class_template_specialization_matcher.cppast_canopy/cpp/src/detail/matchers/enum_matcher.cppast_canopy/cpp/src/detail/matchers/function_matcher.cppast_canopy/cpp/src/detail/matchers/function_template_matcher.cppast_canopy/cpp/src/detail/matchers/record_matcher.cppast_canopy/cpp/src/detail/matchers/typedef_matcher.cppast_canopy/cpp/src/function.cppast_canopy/cpp/src/record.cppast_canopy/cpp/src/template.cppast_canopy/cpp/src/template_param.cppast_canopy/cpp/src/type.cppast_canopy/cpp/src/typedef.cppast_canopy/tests/data/sample_complex_templates.cuast_canopy/tests/test_complex_template_parsing.py
Comment on lines
56
to
+64
| auto &record_id_map = *payload->record_id_to_name; | ||
| record_id_map[id] = rename_for_unnamed; | ||
| payload->decls->records.push_back(Record( | ||
| RD, RecordAncestor::ANCESTOR_IS_NOT_TEMPLATE, rename_for_unnamed)); | ||
| try { | ||
| payload->decls->records.push_back(Record( | ||
| RD, RecordAncestor::ANCESTOR_IS_NOT_TEMPLATE, rename_for_unnamed)); | ||
| } catch (...) { | ||
| // Skip records that cannot be fully processed (e.g. records with | ||
| // dependent child declarations that trigger issues). | ||
| } |
Contributor
There was a problem hiding this comment.
Minor: record_id_map entry persists even when Record construction fails.
The ID is inserted at line 57 before the try block. If Record(...) throws, the map entry remains but the record isn't added to payload->decls->records. This creates a state where record_id_to_name contains entries for records that weren't successfully captured.
While typedef.cpp has a fallback for missing records, this inconsistency could cause confusion during debugging. Consider moving the map insertion inside the try block or removing the entry on failure.
💡 Option: Insert after successful construction
- auto &record_id_map = *payload->record_id_to_name;
- record_id_map[id] = rename_for_unnamed;
try {
+ auto &record_id_map = *payload->record_id_to_name;
+ record_id_map[id] = rename_for_unnamed;
payload->decls->records.push_back(Record(
RD, RecordAncestor::ANCESTOR_IS_NOT_TEMPLATE, rename_for_unnamed));
} catch (...) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| auto &record_id_map = *payload->record_id_to_name; | |
| record_id_map[id] = rename_for_unnamed; | |
| payload->decls->records.push_back(Record( | |
| RD, RecordAncestor::ANCESTOR_IS_NOT_TEMPLATE, rename_for_unnamed)); | |
| try { | |
| payload->decls->records.push_back(Record( | |
| RD, RecordAncestor::ANCESTOR_IS_NOT_TEMPLATE, rename_for_unnamed)); | |
| } catch (...) { | |
| // Skip records that cannot be fully processed (e.g. records with | |
| // dependent child declarations that trigger issues). | |
| } | |
| try { | |
| auto &record_id_map = *payload->record_id_to_name; | |
| record_id_map[id] = rename_for_unnamed; | |
| payload->decls->records.push_back(Record( | |
| RD, RecordAncestor::ANCESTOR_IS_NOT_TEMPLATE, rename_for_unnamed)); | |
| } catch (...) { | |
| // Skip records that cannot be fully processed (e.g. records with | |
| // dependent child declarations that trigger issues). | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ast_canopy/cpp/src/detail/matchers/record_matcher.cpp` around lines 56 - 64,
The map entry for record_id_map (payload->record_id_to_name) is added before
constructing the Record, leaving stale entries if Record(RD, ...) throws; move
the insertion record_id_map[id] = rename_for_unnamed so it happens only after
the Record is successfully created and pushed into payload->decls->records
(i.e., place it after the push_back), or alternatively, keep it before but in
the catch block erase the key (record_id_map.erase(id)) to ensure no lingering
entry; reference record_id_map, payload->record_id_to_name, Record(...),
payload->decls->records, rename_for_unnamed and RD when making the change.
Comment on lines
+41
to
+46
| try { | ||
| payload->decls->typedefs.push_back( | ||
| Typedef(TD, payload->record_id_to_name)); | ||
| } catch (...) { | ||
| // Skip typedefs that cannot be processed. | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Silent exception swallowing hides diagnostic information.
The catch (...) block discards all exception context. While this achieves the crash-prevention goal, consider logging the skipped typedef name (at least in debug builds) to aid troubleshooting when declarations are unexpectedly missing.
try {
payload->decls->typedefs.push_back(
Typedef(TD, payload->record_id_to_name));
} catch (...) {
- // Skip typedefs that cannot be processed.
+ // Skip typedefs that cannot be processed.
+#ifndef NDEBUG
+ std::cerr << "Warning: skipping typedef '" << name
+ << "' due to exception during construction\n";
+#endif
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ast_canopy/cpp/src/detail/matchers/typedef_matcher.cpp` around lines 41 - 46,
The catch-all in typedef_matcher.cpp silently swallows exceptions when pushing
Typedef into payload->decls->typedefs; modify the exception handling in the
try/catch around Typedef(TD, payload->record_id_to_name) to catch std::exception
(or catch and rethrow after logging) and log the typedef identifier/name
(obtainable from TD or via payload->record_id_to_name) on failure—at minimum
emit a debug-level message with the typedef name and exception.what(), so only
non-fatal failures are skipped but diagnostics are available for debugging.
Comment on lines
+79
to
+80
| if (has_dependent_signature(FD)) { | ||
| mangled_name = qual_name; // best-effort fallback |
Contributor
There was a problem hiding this comment.
Keep the dependent fallback overload-safe.
Because this branch keeps only qual_name, every dependent overload collapses to the same mangled_name. X<T>::f(int) and X<T>::f(double) will both serialize as X::f, so the fallback needs to incorporate the spelled function type instead of dropping the parameter list.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ast_canopy/cpp/src/function.cpp` around lines 79 - 80, The
dependent-signature fallback currently assigns mangled_name = qual_name in
has_dependent_signature(FD), collapsing all dependent overloads; instead append
or incorporate the spelled function type (the parameter list) into mangled_name
so overloads remain distinct (e.g., use FD's spelled/printed function type or
parameter-type-string and concatenate it to qual_name), updating the branch
where has_dependent_signature(FD) is handled to produce qual_name +
spelledFunctionType (or a deterministically formatted parameter-list string)
rather than just qual_name.
Comment on lines
+31
to
+38
| // The underlying record was not captured by the record matcher (e.g. it | ||
| // is a class template instantiation, comes from a non-retained file, or | ||
| // lives inside a partial specialization). Fall back to the name Clang | ||
| // gives us directly. | ||
| underlying_name = underlying_record_decl->getNameAsString(); | ||
| if (underlying_name.empty()) { | ||
| underlying_name = underlying_record_decl->getQualifiedNameAsString(); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the typedef.cpp file
find . -name "typedef.cpp" -path "*/cpp/src/*" | head -5Repository: NVIDIA/numbast
Length of output: 90
🏁 Script executed:
# Let's also check the repository structure to understand the codebase
git ls-files | grep -E "(typedef|template)" | head -20Repository: NVIDIA/numbast
Length of output: 1077
🏁 Script executed:
# Examine the typedef.cpp file around lines 31-38
cat -n ast_canopy/cpp/src/typedef.cpp | head -50Repository: NVIDIA/numbast
Length of output: 2100
🏁 Script executed:
# Look at the full context of the Typedef::Typedef constructor to understand the fallback logic
wc -l ast_canopy/cpp/src/typedef.cppRepository: NVIDIA/numbast
Length of output: 91
🏁 Script executed:
# Check how ClassTemplateSpecializationDecl is handled elsewhere
grep -r "ClassTemplateSpecializationDecl" ast_canopy/cpp/src/ -A 3 -B 3Repository: NVIDIA/numbast
Length of output: 1723
🏁 Script executed:
# Look at the class_template_specialization.cpp to understand how template specializations are handled
cat -n ast_canopy/cpp/src/class_template_specialization.cpp | head -80Repository: NVIDIA/numbast
Length of output: 3538
🏁 Script executed:
# Check the test file for typedef to understand the expected behavior
cat -n ast_canopy/tests/data/sample_typedef.cuRepository: NVIDIA/numbast
Length of output: 403
🏁 Script executed:
# Search for typedef usage with template specializations in tests
grep -r "typedef.*<" ast_canopy/tests/ --include="*.cu" --include="*.py"Repository: NVIDIA/numbast
Length of output: 238
🏁 Script executed:
# Check if there's a test for typedef of template specializations
grep -r "Typedef" ast_canopy/tests/ --include="*.py" -A 5 | head -40Repository: NVIDIA/numbast
Length of output: 1735
🏁 Script executed:
# Look for how the underlying_name is used after this fallback
grep -r "underlying_name" ast_canopy/ --include="*.cpp" --include="*.hpp"Repository: NVIDIA/numbast
Length of output: 1369
🏁 Script executed:
# Examine the test_complex_template_parsing.py to understand what's expected
cat -n ast_canopy/tests/test_complex_template_parsing.pyRepository: NVIDIA/numbast
Length of output: 6520
🏁 Script executed:
# Check the sample_complex_templates.cu file more carefully
cat -n ast_canopy/tests/data/sample_complex_templates.cuRepository: NVIDIA/numbast
Length of output: 4392
🏁 Script executed:
# Search for how getAsString() works with template specializations in Clang context
# Let's check if there's documentation or comments about QualType spelling
grep -r "getAsString" ast_canopy/cpp/ -B 2 -A 2 | head -60Repository: NVIDIA/numbast
Length of output: 1674
🏁 Script executed:
# Let's verify the Clang API behavior for ClassTemplateSpecializationDecl
# by checking if there's any usage pattern in the codebase showing how
# template specializations are handled when printing their names
grep -r "getNameAsString\|getQualifiedNameAsString" ast_canopy/cpp/ -B 3 -A 3 | grep -A 5 -B 5 "specialization\|CTSD\|ClassTemplate"Repository: NVIDIA/numbast
Length of output: 2572
🏁 Script executed:
# Check if underlying_name from the typedef is used downstream
# to understand the impact of this change
grep -r "\.underlying_name" ast_canopy/ --include="*.cpp" --include="*.py" -B 2 -A 2Repository: NVIDIA/numbast
Length of output: 2105
🏁 Script executed:
# Check if the test_typedef_to_template_instantiation test actually validates
# the underlying_name values
cat -n ast_canopy/tests/test_complex_template_parsing.py | sed -n '88,110p'Repository: NVIDIA/numbast
Length of output: 1008
🏁 Script executed:
# Look at what the current implementation returns for the test case
# by checking if there's a debug output or assertion on underlying_name
grep -A 20 "test_typedef_to_template_instantiation" ast_canopy/tests/test_complex_template_parsing.pyRepository: NVIDIA/numbast
Length of output: 806
🏁 Script executed:
# Check if QualType has getAsString() method available and used elsewhere
grep -r "QualType.*getAsString\|qd\.getAsString" ast_canopy/cpp/ -B 2 -A 2Repository: NVIDIA/numbast
Length of output: 393
Use underlying QualType spelling to preserve template specialization arguments.
For ClassTemplateSpecializationDecl, getNameAsString() and getQualifiedNameAsString() omit template arguments, so Storage<float, 3> degrades to Storage. Instead use qd.getAsString() to preserve the full type spelling with specialization arguments, consistent with the non-record fallback (line 43) and avoiding the typedef-to-instantiation ambiguity this PR fixes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ast_canopy/cpp/src/typedef.cpp` around lines 31 - 38, The current fallback
uses underlying_record_decl->getNameAsString()/getQualifiedNameAsString(), which
drops template arguments for ClassTemplateSpecializationDecl; instead obtain the
QualType spelling and assign underlying_name from qd.getAsString() (the same
spelling used on line 43) so specializations like Storage<float, 3> keep their
template arguments; update the code around underlying_record_decl and
underlying_name to use the QualType (qd) spelling (qd.getAsString()) when
forming underlying_name, preserving template specialization arguments for
ClassTemplateSpecializationDecl/TemplateSpecializationType.
Comment on lines
+41
to
+85
| def test_parse_crtp_base_does_not_crash(self, source_path): | ||
| """Parsing a header with CRTP base classes should not segfault.""" | ||
| decls = parse_declarations_from_source( | ||
| source_path, | ||
| [source_path], | ||
| "sm_80", | ||
| bypass_parse_error=True, | ||
| ) | ||
| assert decls is not None | ||
|
|
||
| def test_crtp_class_templates_found(self, source_path): | ||
| """CRTP base templates should be reported as class templates.""" | ||
| decls = parse_declarations_from_source( | ||
| source_path, | ||
| [source_path], | ||
| "sm_80", | ||
| bypass_parse_error=True, | ||
| ) | ||
| ct_names = [ct.qual_name for ct in decls.class_templates] | ||
| assert any("CRTPBase" in n for n in ct_names) | ||
| assert any("Vec3" in n for n in ct_names) | ||
|
|
||
| def test_deep_crtp_hierarchy(self, source_path): | ||
| """Multi-level CRTP (Level1 -> Level2 -> Concrete) should parse.""" | ||
| decls = parse_declarations_from_source( | ||
| source_path, | ||
| [source_path], | ||
| "sm_80", | ||
| bypass_parse_error=True, | ||
| ) | ||
| ct_names = [ct.qual_name for ct in decls.class_templates] | ||
| assert any("Level1" in n for n in ct_names) | ||
| assert any("Level2" in n for n in ct_names) | ||
| assert any("Concrete" in n for n in ct_names) | ||
|
|
||
| def test_concrete_function_still_parsed(self, source_path): | ||
| """Non-template functions using CRTP types should still parse.""" | ||
| decls = parse_declarations_from_source( | ||
| source_path, | ||
| [source_path], | ||
| "sm_80", | ||
| bypass_parse_error=True, | ||
| ) | ||
| func_names = [f.name for f in decls.functions] | ||
| assert "vec3_dot" in func_names |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider caching parsed declarations to avoid repeated parsing.
Each test method calls parse_declarations_from_source independently. Since the source file is the same and tests are read-only, parsing once in a class-scoped fixture would speed up the test suite.
♻️ Suggested optimization using class-scoped fixture
class TestCRTPDependentTypes:
"""Case 1 & 5: CRTP hierarchies with dependent method types."""
`@pytest.fixture`(scope="class")
def decls(self, source_path):
return parse_declarations_from_source(
source_path,
[source_path],
"sm_80",
bypass_parse_error=True,
)
def test_parse_crtp_base_does_not_crash(self, decls):
assert decls is not None
def test_crtp_class_templates_found(self, decls):
ct_names = [ct.qual_name for ct in decls.class_templates]
assert any("CRTPBase" in n for n in ct_names)
assert any("Vec3" in n for n in ct_names)
# ... etc🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ast_canopy/tests/test_complex_template_parsing.py` around lines 41 - 85, The
tests repeatedly call parse_declarations_from_source causing redundant parsing;
create a class-scoped pytest fixture (e.g., def decls(self, source_path): return
parse_declarations_from_source(source_path, [source_path], "sm_80",
bypass_parse_error=True)) and update each test method in the class to accept
that fixture (replace local calls to parse_declarations_from_source with using
the decls variable), keeping unique assertions (use decls.class_templates and
decls.functions) so parsing runs once per test class.
Comment on lines
+95
to
+96
| def test_typedef_to_template_instantiation(self, source_path): | ||
| """Typedefs like 'using Vec3fStorage = Storage<float, 3>' should parse.""" |
Contributor
There was a problem hiding this comment.
Minor: Docstring mentions using but test data uses typedef.
The docstring says 'using Vec3fStorage = Storage<float, 3>' but sample_complex_templates.cu line 37 uses typedef Storage<float, 3> Vec3fStorage;. Consider updating for accuracy.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ast_canopy/tests/test_complex_template_parsing.py` around lines 95 - 96, The
test docstring for test_typedef_to_template_instantiation is inaccurate: it
mentions a C++ using-declaration but the test uses a typedef form; update the
docstring in test_typedef_to_template_instantiation to reflect the actual sample
(e.g., "Typedefs like 'typedef Storage<float, 3> Vec3fStorage;' should parse.")
or alternatively change the test input to use the using form so the docstring
matches; ensure the text references Vec3fStorage and Storage<float, 3> to keep
intent clear.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ast_canopyparses headers with deep template metaprogramming (CRTP hierarchies, dependent types, template template parameters, expression-based template arguments)Root causes fixed
mangleName()segfaults on dependent (uninstantiated) template types — methods inside class templates have dependent parameter/return types that the Itanium mangler can't handle. Addedhas_dependent_signature()to skip mangling. Also fixed a memory leak (ItaniumMangleContextnow usesunique_ptr).getCanonicalType()on dependent types crashes. Added null-QualType guard and skip canonicalization for dependent types.getTypeSize()/getTypeAlign()crash on dependent/incomplete types. Added guards and try-catch around child declaration construction.record_id_to_name->at()throws for template instantiations not in the map. Replaced with safefind()+ fallback. Added null guard forgetAsCXXRecordDecl().TemplateTemplateParmDeclconstructor threw. Now handles withkind=template_."<unsupported>"placeholder.Test plan
test_complex_template_parsing.pywith 7 tests covering CRTP, typedefs to template instantiations, template template parameters, expression-based template args, and deep inheritance hierarchies🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests