Open
Conversation
bind_cxx_struct gains an optional keyword `name=` that overrides struct_decl.name throughout: the Numba type name, the Python API class name, the CTYPE_MAPS registration key, and the shim template substitutions. When binding a class template specialization, struct_decl.name is just the unqualified template name (e.g. "Matrix"), but the shim code and type registry both need the fully qualified specialization name (e.g. "Namespace::Matrix<float, 3, 1>"). The override is plumbed through bind_cxx_struct_ctors, bind_cxx_struct_regular_methods, and bind_cxx_struct_conversion_opeartors so every downstream consumer sees the same name. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughAdds an optional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
numbast/src/numbast/struct.py (1)
203-206:⚠️ Potential issue | 🔴 CriticalSanitize conversion-operator shim symbol names when
name=contains C++ syntax.At line 204,
mangled_nameembeds rawstruct_name. Thebind_cxx_structs()API explicitly documents thatnamecan be a fully qualified template specialization (e.g.,"Eigen::Matrix<float,3,1>"), which contains characters (:,<,>,,, space) invalid in C/C++ function identifiers. Generated shim symbols fail to compile when conversion operators are present.The
deduplicate_overloads()function applies no sanitization—it only appends a counter suffix. Sanitizestruct_namebefore embedding it in the symbol:Proposed fix
- mangled_name = deduplicate_overloads( - f"__{struct_name}__{conv.mangled_name}" - ) + safe_struct_token = "".join( + ch if (ch.isalnum() or ch == "_") else "_" + for ch in struct_name + ) + mangled_name = deduplicate_overloads( + f"__{safe_struct_token}__{conv.mangled_name}" + )Also applies to: 231-241 (wrapper function
bind_cxx_struct_conversion_opeartorspasses unsanitizedstruct_nametobind_cxx_struct_conversion_opeartor).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@numbast/src/numbast/struct.py` around lines 203 - 206, The generated shim symbol embeds raw struct_name (e.g., "Eigen::Matrix<float,3,1>") which contains characters invalid in C/C++ identifiers; sanitize struct_name before composing mangled_name/shim_func_name in bind_cxx_struct_conversion_opeartor (and wherever bind_cxx_struct_conversion_opeartors forwards struct_name to it). Fix by normalizing struct_name to a valid identifier (e.g., replace any character not in [A-Za-z0-9_] with '_' or convert "::" to "__") via a small helper (sanitize_identifier or sanitize_symbol) and use that sanitized name when building f"__{struct_name}__{conv.mangled_name}" and shim_func_name; also pass the sanitized struct_name into bind_cxx_struct_conversion_opeartor from bind_cxx_struct_conversion_opeartors so all locations use the safe symbol name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@numbast/src/numbast/struct.py`:
- Around line 479-482: The alias registration only looks up aliases by
struct_name and thus can miss aliases keyed by the parsed declaration name (e.g.
struct_decl.name) and overwrite existing mappings; update the block that sets
C2N to (1) gather alias lists from both aliases.get(struct_name, []) and
aliases.get(struct_decl.name, []) (or the original parsed name variable) and (2)
when iterating those alias names, set C2N[alias] = s_type only if the alias key
is not already present (or explicitly merge preserving existing entries) so
existing alias mappings aren’t dropped; reference variables: C2N, struct_name,
aliases, s_type, and struct_decl.name.
In `@numbast/tests/test_bind_cxx_struct_name_override.py`:
- Around line 44-70: Both tests mutate the global CTYPE_MAPS via bind_cxx_struct
(affecting keys like "Vec" and the override string) and never undo that change;
update each test (test_bind_without_name_uses_struct_decl_name and
test_bind_with_name_uses_override_everywhere) to isolate side effects by saving
any existing entries for the relevant keys (CTYPE_MAPS.get("Vec") and
CTYPE_MAPS.get(override)), run bind_cxx_struct and assertions, then restore the
original mapping (or pop the newly added keys) in a finally block so CTYPE_MAPS
is returned to its prior state; also ensure to_numba_type lookups remain valid
during the test before cleanup.
---
Outside diff comments:
In `@numbast/src/numbast/struct.py`:
- Around line 203-206: The generated shim symbol embeds raw struct_name (e.g.,
"Eigen::Matrix<float,3,1>") which contains characters invalid in C/C++
identifiers; sanitize struct_name before composing mangled_name/shim_func_name
in bind_cxx_struct_conversion_opeartor (and wherever
bind_cxx_struct_conversion_opeartors forwards struct_name to it). Fix by
normalizing struct_name to a valid identifier (e.g., replace any character not
in [A-Za-z0-9_] with '_' or convert "::" to "__") via a small helper
(sanitize_identifier or sanitize_symbol) and use that sanitized name when
building f"__{struct_name}__{conv.mangled_name}" and shim_func_name; also pass
the sanitized struct_name into bind_cxx_struct_conversion_opeartor from
bind_cxx_struct_conversion_opeartors so all locations use the safe symbol name.
🪄 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: b2089a07-ef2c-43c4-bc1d-3fdbc820c1ad
📒 Files selected for processing (3)
numbast/src/numbast/struct.pynumbast/tests/data/sample_name_override.cuhnumbast/tests/test_bind_cxx_struct_name_override.py
Comment on lines
+479
to
482
| C2N[struct_name] = s_type | ||
| if struct_name in aliases: | ||
| for alias in aliases[struct_name]: | ||
| C2N[alias] = s_type |
Contributor
There was a problem hiding this comment.
Alias registration keyed only by override name can drop existing alias mappings.
When name is provided, aliases keyed by struct_decl.name are no longer applied because lookup now checks only struct_name. That is a behavior regression for callers that keep alias maps keyed by parsed names.
🔧 Proposed fix
C2N[struct_name] = s_type
- if struct_name in aliases:
- for alias in aliases[struct_name]:
- C2N[alias] = s_type
+ alias_keys = [struct_name]
+ if struct_decl.name != struct_name:
+ alias_keys.append(struct_decl.name)
+ for key in alias_keys:
+ for alias in aliases.get(key, []):
+ C2N[alias] = s_type🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@numbast/src/numbast/struct.py` around lines 479 - 482, The alias registration
only looks up aliases by struct_name and thus can miss aliases keyed by the
parsed declaration name (e.g. struct_decl.name) and overwrite existing mappings;
update the block that sets C2N to (1) gather alias lists from both
aliases.get(struct_name, []) and aliases.get(struct_decl.name, []) (or the
original parsed name variable) and (2) when iterating those alias names, set
C2N[alias] = s_type only if the alias key is not already present (or explicitly
merge preserving existing entries) so existing alias mappings aren’t dropped;
reference variables: C2N, struct_name, aliases, s_type, and struct_decl.name.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
numbast/tests/test_bind_cxx_struct_name_override.py (1)
42-72:⚠️ Potential issue | 🟡 MinorIsolate
CTYPE_MAPSmutations to prevent cross-test coupling.Both binding tests mutate global
CTYPE_MAPSand leave entries behind ("Vec"/ override key). This can make suite order affect outcomes. Save prior values and restore them infinally.Proposed isolation patch
def test_bind_without_name_uses_struct_decl_name(): @@ - S = bind_cxx_struct(shim_writer, cts, nbtypes.Type, StructModel) - - assert S._nbtype.name == "Vec" - assert CTYPE_MAPS.get("Vec") is S._nbtype - assert to_numba_type("Vec") is S._nbtype + key = "Vec" + had_prev = key in CTYPE_MAPS + prev = CTYPE_MAPS.get(key) + try: + S = bind_cxx_struct(shim_writer, cts, nbtypes.Type, StructModel) + assert S._nbtype.name == key + assert CTYPE_MAPS.get(key) is S._nbtype + assert to_numba_type(key) is S._nbtype + finally: + if had_prev: + CTYPE_MAPS[key] = prev + else: + CTYPE_MAPS.pop(key, None) @@ def test_bind_with_name_uses_override_everywhere(): @@ - S = bind_cxx_struct( - shim_writer, - cts, - nbtypes.Type, - StructModel, - name=override, - ) - - assert S._nbtype.name == override - assert S.__name__ == override - assert CTYPE_MAPS.get(override) is S._nbtype - assert to_numba_type(override) is S._nbtype + key = override + had_prev = key in CTYPE_MAPS + prev = CTYPE_MAPS.get(key) + try: + S = bind_cxx_struct( + shim_writer, + cts, + nbtypes.Type, + StructModel, + name=override, + ) + assert S._nbtype.name == key + assert S.__name__ == key + assert CTYPE_MAPS.get(key) is S._nbtype + assert to_numba_type(key) is S._nbtype + finally: + if had_prev: + CTYPE_MAPS[key] = prev + else: + CTYPE_MAPS.pop(key, None)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@numbast/tests/test_bind_cxx_struct_name_override.py` around lines 42 - 72, The tests mutate the global CTYPE_MAPS via bind_cxx_struct and leave keys ("Vec" and the override) set, causing cross-test coupling; modify both test_bind_without_name_uses_struct_decl_name and test_bind_with_name_uses_override_everywhere to snapshot CTYPE_MAPS.get(key) for the relevant key(s) before calling bind_cxx_struct and restore the original value (or delete the key if it was absent) in a try/finally block after the assertions; reference CTYPE_MAPS, bind_cxx_struct, S._nbtype.name, and the override string to locate where to save/restore the prior mapping so the global state is unchanged after each test runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@numbast/tests/test_bind_cxx_struct_name_override.py`:
- Around line 42-72: The tests mutate the global CTYPE_MAPS via bind_cxx_struct
and leave keys ("Vec" and the override) set, causing cross-test coupling; modify
both test_bind_without_name_uses_struct_decl_name and
test_bind_with_name_uses_override_everywhere to snapshot CTYPE_MAPS.get(key) for
the relevant key(s) before calling bind_cxx_struct and restore the original
value (or delete the key if it was absent) in a try/finally block after the
assertions; reference CTYPE_MAPS, bind_cxx_struct, S._nbtype.name, and the
override string to locate where to save/restore the prior mapping so the global
state is unchanged after each test runs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 147a4fec-08fd-4f7f-9499-1f52cc05da9b
📒 Files selected for processing (1)
numbast/tests/test_bind_cxx_struct_name_override.py
|
The conversion-operator bind path composes the shim's extern "C" function name from struct_name. With the new name= override, callers may pass a fully-qualified template specialization (e.g. "Eigen::Matrix<float, 3, 1>"), which contains ':', '<', '>', ',', and spaces — all invalid in a C identifier and rejected by nvrtc. Sanitize struct_name (any char outside [A-Za-z0-9_] -> '_') before interpolating into mangled_name/shim_func_name. The C++ type reference inside the shim body still uses the original struct_name. Also snapshot and restore CTYPE_MAPS around the name-override tests so bind_cxx_struct side effects don't leak between tests. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
numbast/src/numbast/struct.py (1)
495-498:⚠️ Potential issue | 🟠 MajorAlias registration should consult both override and parsed struct name.
Line 496 only checks aliases under
struct_name. Withname=..., aliases keyed bystruct_decl.nameare skipped, and Line 498 can also clobber an existing alias mapping.🔧 Proposed fix
C2N[struct_name] = s_type - if struct_name in aliases: - for alias in aliases[struct_name]: - C2N[alias] = s_type + alias_keys = [struct_name] + if struct_decl.name != struct_name: + alias_keys.append(struct_decl.name) + for key in alias_keys: + for alias in aliases.get(key, []): + C2N.setdefault(alias, s_type)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@numbast/src/numbast/struct.py` around lines 495 - 498, The alias registration currently only uses struct_name and can skip aliases keyed by the parsed declaration name and may overwrite existing mappings; update the logic around C2N, struct_name, aliases, name and struct_decl.name so you collect aliases for both the effective struct_name and the original struct_decl.name (if different), iterate both alias lists and set C2N[alias] = s_type only when an alias is not already present (i.e., avoid clobbering an existing mapping), ensuring you still set C2N[struct_name] = s_type for the effective name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@numbast/tests/test_bind_cxx_struct_name_override.py`:
- Around line 24-32: The autouse fixture _restore_ctype_maps should be removed
and its behavior inlined into each test that mutates CTYPE_MAPS: for every test
that calls bind_cxx_struct or otherwise mutates CTYPE_MAPS, take a local
snapshot (snapshot = dict(CTYPE_MAPS)) at the start and restore it in a finally
block (CTYPE_MAPS.clear(); CTYPE_MAPS.update(snapshot)) at the end of that test
so each test manages its own setup/teardown and can evolve independently.
---
Duplicate comments:
In `@numbast/src/numbast/struct.py`:
- Around line 495-498: The alias registration currently only uses struct_name
and can skip aliases keyed by the parsed declaration name and may overwrite
existing mappings; update the logic around C2N, struct_name, aliases, name and
struct_decl.name so you collect aliases for both the effective struct_name and
the original struct_decl.name (if different), iterate both alias lists and set
C2N[alias] = s_type only when an alias is not already present (i.e., avoid
clobbering an existing mapping), ensuring you still set C2N[struct_name] =
s_type for the effective name.
🪄 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: Enterprise
Run ID: cc77d48e-e10b-4d40-bf0e-635c581611ea
📒 Files selected for processing (3)
numbast/src/numbast/struct.pynumbast/tests/data/sample_name_override.cuhnumbast/tests/test_bind_cxx_struct_name_override.py
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
name=tobind_cxx_structthat overridesstruct_decl.namethroughout: the Numba type name, the Python API class name, theCTYPE_MAPSregistration key, and the shim template substitutions.struct_decl.nameis just the unqualified template name (e.g."Matrix"), but the shim code and type registry both need the fully qualified specialization name (e.g."Namespace::Matrix<float, 3, 1>").bind_cxx_struct_ctors,bind_cxx_struct_regular_methods, andbind_cxx_struct_conversion_opeartorsso every downstream consumer sees the same name.Test plan
numbast/tests/test_bind_cxx_struct_name_override.pyagainst a fresh stub header with ademo::Vec<float,3>specialization:struct_decl.nameon the specialization is"Vec"(motivates the override).name=, the Numba type name andCTYPE_MAPSkey use the unqualified specialization name.name=, the override drives Numba type name, Python API class name, andCTYPE_MAPSregistration key.pytest numbast/tests/test_bind_cxx_struct_name_override.pypasses locally.Summary by CodeRabbit
New Features
Tests