Dry consolidation - #120
Merged
Merged
Conversation
The "is this method/constructor/data member wrapped?" decision was stated twice for each kind - once in a writer (emits the binding) and once inline in PackageInfo._iter_wrapped_arg_return_types (yields the arg/return/member types for dependency pruning and auto-includes). The duplication was enforced only by a comment and tests, and the inline walk had silently drifted: it lacked the writers' parent / private / artificial-copy-constructor / private-pure-virtual exclusions. Add cppwg/info/exclusions.py with method_is_excluded, constructor_is_excluded and variable_exclusion_reason / variable_is_excluded as the single source. The three writers delegate to it (the member writer keeps its per-reason debug logs via the reason helper) and the info walk calls the same predicates, so the two can no longer diverge. It lives in the info layer (imports only utils + pygccxml) to respect the info-must-not-import-writers boundary. Behaviour-preserving: the shapes wrappers regenerate byte-identically and the example builds and passes. Reorder the artificial-copy-constructor check so the cheap is_artificial gate short-circuits the heavier is_copy_constructor call. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The shared (BaseInfo-level) config options were restated in three places:
BaseInfo's hand-written attribute defaults, BaseInfo's copy-from-config key
list, and the parser's base_config seed dict. Adding an option to one and
forgetting another silently ignored it - which is exactly how name_replacements
became unreachable: it was a BaseInfo option with a default map, but the
parser's seed omitted it, so a user's name_replacements: in the YAML was
dropped.
Introduce BASE_INFO_OPTIONS, one {option: default} schema, as the single
source. BaseInfo seeds its defaults and copies overrides from it (deep-copying
each default so info objects no longer alias a shared list/dict), and the
parser builds base_config from it. name_replacements is now in the schema, so
package- and class-level values are honoured (+2 regression tests).
exclude_inherited_overrides stays special (its per-level False/None cascade
must not get one shared default), noted via _EXTRA_CONFIG_KEYS.
Behaviour-preserving: the shapes wrappers regenerate byte-identically and the
example builds and passes.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The ', py::arg("name") [= value]' fragment was rendered by three near-identical
loops in the method, constructor and free-function writers (str_to_num
normalisation, class-template-parameter substitution, and the constructor-only
empty-initializer-list typing). The free-function copy had drifted: it gated
the whole loop on exclude_default_args, so with the flag set it dropped the
py::arg("name") keyword names too - unlike the method/constructor writers,
which the docs match by omitting only the default values.
Add CppBaseWrapperWriter.render_default_args as the single source (template
substitution and empty-init-list typing behind parameters). The three writers
call it; method/constructor output is byte-identical and free functions now
keep their py::arg names when exclude_default_args is set. Drop the now-unused
re/utils/type_traits imports from the method and constructor writers.
Behaviour-preserving for the examples (they do not set exclude_default_args):
the shapes wrappers regenerate byte-identically and the example builds/passes.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The .value("NAME", Scope::NAME) lines were rendered by near-identical loops in
the enum writer and the struct-enum class path, and the ".export_values() vs ;"
decision lived in CppEnumInfo.should_export_values but was bypassed by the
struct-enum template, which hardcoded .export_values(). So a scoped enum class
nested in a wrapped struct emitted wrong C++ - exporting enumerators it should
not.
Add utils.render_enum_value_lines (the value lines) and
utils.should_export_enum_values (the decision rule). The enum writer and
CppEnumInfo.should_export_values use them, and build_struct_enum_register now
renders the value lines and computes its terminator through the same rule; the
struct_enum_register template takes a ${enum_terminator} instead of a hardcoded
.export_values(). A scoped nested enum now closes with `;` (+1 regression test).
Behaviour-preserving for the examples (their enums are unscoped): the shapes
wrappers regenerate byte-identically and the example builds and passes.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The register_<py_name>_class affix was hand-typed in four places: the module
main cpp's call (an f-string in module_writer) and the definition/declaration in
three templates (class_cpp_register, struct_enum_register,
class_hpp_register_declaration). Drift between the call and a definition is an
undefined-symbol link error.
Add utils.registration_function_name as the single source. The three templates
take a ${register_function} placeholder fed from it, and module_writer builds
the call with it, so the affix is defined once.
Behaviour-preserving: the shapes wrappers regenerate byte-identically and the
example builds and passes.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The {py_name_base}.cppwg.hpp / .cpp filename was composed in four places: the
files written (class_writer.write_files), the module's #include of a class
wrapper (module_writer), and the cpp's own #include (class_cpp_header template).
Drift is a missing-header compile error.
Add CppClassInfo.wrapper_header_filename / wrapper_source_filename as the single
source. write_files and the module include use them, and the class_cpp_header
template takes the filename via a ${class_hpp_filename} placeholder fed from the
same method. write_files no longer needs its file_stem argument (always the
class's own py_name_base). The module-main filename and the include guard are
different patterns and left as they are.
Behaviour-preserving: the shapes wrappers regenerate byte-identically and the
example builds and passes.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The `name.split("::")[-1]` idiom - reduce a (possibly qualified) name to its
unqualified form to match pygccxml's unqualified declaration names - was
open-coded in four places (class_writer bases_block, source_parser,
package_info, and utils itself) alongside class_info._unqualified_base_name,
which additionally strips template arguments.
Add utils.unqualified_name as the single source for the namespace strip. The
four sites use it, and _unqualified_base_name composes it with a leading
template-argument strip. Behaviour is unchanged (the four sites operate on
names that carry no template arguments, or intentionally keep them).
The `split("<")[0]` template-argument strip (generators, and the leading step
of _unqualified_base_name) is a distinct operation and left as-is. Shapes
wrappers regenerate byte-identically; example builds and passes.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
py_name_base (the class base name) and update_py_names (each template arg) mangled C++ tokens into Python-name-safe fragments with near-identical code that had diverged only on `<`/`,`: the base name removed them, while a template arg turned them into `_` so a nested template stays readable. Verified that divergence is deliberate, not a bug. Extract _mangle_py_token(text, separator): it applies name_replacements, turns `<`/`,` into the separator, removes `>`/spaces and capitalises. py_name_base passes separator="" and update_py_names passes "_", so the shared logic lives in one place and the intentional difference is a single explicit argument (+1 test pinning both). Byte-identical: shapes wrappers regenerate unchanged and the example builds and passes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #120 +/- ##
===========================================
+ Coverage 99.69% 99.73% +0.04%
===========================================
Files 28 29 +1
Lines 2282 2256 -26
Branches 507 490 -17
===========================================
- Hits 2275 2250 -25
+ Misses 6 5 -1
Partials 1 1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Consolidates duplicated binding-generation logic to prevent writer, parser, and dependency-analysis drift.
Changes:
- Centralizes exclusion predicates, default-argument rendering, naming, filenames, and enum rendering.
- Derives parser defaults from a shared configuration schema.
- Adds regression tests for prior inconsistencies.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
cppwg/info/base_info.py |
Defines shared configuration defaults. |
cppwg/info/class_info.py |
Consolidates name mangling and wrapper filenames. |
cppwg/info/enum_info.py |
Shares enum export logic. |
cppwg/info/exclusions.py |
Centralizes member exclusion predicates. |
cppwg/info/package_info.py |
Reuses exclusions during dependency discovery. |
cppwg/parsers/package_info_parser.py |
Seeds configuration from the shared schema. |
cppwg/parsers/source_parser.py |
Reuses unqualified-name handling. |
cppwg/templates/pybind11_default.py |
Uses consolidated template substitutions. |
cppwg/utils/utils.py |
Adds shared naming and enum helpers. |
cppwg/writers/base_writer.py |
Centralizes default-argument rendering. |
cppwg/writers/class_writer.py |
Reuses naming, filename, and enum helpers. |
cppwg/writers/constructor_writer.py |
Reuses exclusions and argument rendering. |
cppwg/writers/enum_writer.py |
Reuses enum-value rendering. |
cppwg/writers/free_function_writer.py |
Preserves argument names when omitting defaults. |
cppwg/writers/member_variable_writer.py |
Reuses variable exclusions. |
cppwg/writers/method_writer.py |
Reuses exclusions and argument rendering. |
cppwg/writers/module_writer.py |
Reuses registration and filename helpers. |
tests/test_class_info.py |
Tests shared name mangling. |
tests/test_class_writer.py |
Tests scoped nested enums and filename helpers. |
tests/test_constructor_writer.py |
Updates exclusion mocks and formatting. |
tests/test_free_function_writer.py |
Tests default-value suppression behavior. |
tests/test_module_writer.py |
Updates wrapper filename test doubles. |
tests/test_package_info.py |
Updates shared-exclusion test doubles. |
tests/test_package_info_parser.py |
Tests parsed name replacements and defaults. |
tests/test_utils.py |
Tests naming helpers. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…ults The parser shallow-copies one base_config into every package/module/class config, so the same list/dict object reaches many info objects. BaseInfo deep-copied the schema defaults but then assigned the config value by reference, re-sharing it - so e.g. every class's excluded_methods / name_replacements was the same object, and a later mutation of one would leak to its siblings and parent. Deep-copy each config value on assignment (+ regression test). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two correctness fixes to code added in this branch, both in utils and its
callers (they share utils.py, so they land together):
unqualified_name used rsplit("::", 1), which selects the last :: even inside a
template argument: unqualified_name("foo::Bar<std::vector<int>>") returned
"vector<int>>" instead of "Bar<std::vector<int>>" (and could stop a templated
external_bases entry matching). It now finds the last :: at template-nesting
depth zero.
is_scoped_enum_in_source_file scanned the whole file for `enum class <name>`, so
two same-named enums with different scopedness in one file (a scoped A::Value
and an unscoped B::Value) confused each other. It now checks only the enum's own
declaration line, from the decl's location.line, which both callers (the plain
enum path and the struct-enum path) now pass.
Both behaviour-preserving on the examples: shapes wrappers regenerate
byte-identically (ShapeKind still exports, Handedness still does not) and the
example builds and passes. +tests for both.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 26 changed files in this pull request and generated no new comments.
Suppressed comments (1)
cppwg/utils/utils.py:445
- This only inspects one physical line, so valid split declarations such as
enum class\nValue { ... }(and declarations with block comments between tokens) are misclassified as unscoped. That makes the generated wrapper emit.export_values()despite the enum being scoped. Scan forward from the reported declaration line, strip comments, and classify the first same-named enum match; please also cover a multiline declaration in the regression test.
line = lines[line_number - 1].split("//", 1)[0]
kwabenantim
marked this pull request as ready for review
August 11, 2026 10:09
The previous line-based check inspected only one physical line, so a scoped enum whose declaration is split across lines (enum class\nValue) or broken by a block comment (enum /* */ class Value) was misclassified as unscoped, wrongly emitting .export_values(). pygccxml also reports the line of the enum *name*, which can sit below the enum keyword, so scanning forward from it is not enough. Match the whole comment-stripped file (comments blanked line-preservingly so a match still maps to its source line) for every `enum [class|struct] <name>` declaration - \s+ spans the newlines/blanked comments of a split declaration - and classify the one whose name sits nearest the reported line. This keeps the same-named-enum disambiguation while tolerating split/comment-broken declarations. Regression tests cover split, block-comment, absent-enum and unreadable-file cases; shapes wrappers regenerate byte-identically. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add unit tests for #119 code paths that no coverage flag (unit, shapes, cells) exercised, which had pushed combined coverage below 99%: - exclusions.constructor_is_excluded: an abstract class with only non-abstract bases keeps its constructor, and a constructor_signature_excludes entry of the same arity but different types does not exclude. - base_writer.render_default_args: keyword/value rendering, class template parameter substitution (including a default that references no parameter), and the empty-initializer-list typing. These bring cppwg/info/exclusions.py and cppwg/writers/base_writer.py to 100%. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
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.
Fixes #119