Consolidate templated wrappers#94
Open
kwabenantim wants to merge 11 commits into
Open
Conversation
cppwg emitted one wrapper .hpp/.cpp pair per template instantiation, so a class
wrapped at N instantiations produced 2N files of near-identical code. Emit a
single {Class}.cppwg.hpp/.cpp per class instead, holding one shared preamble and
one register_{py_name}_class function per instantiation. The registration bodies
are unchanged (byte-identical), just co-located.
- Split the class_cpp template into class_cpp_header (the once-per-class
preamble: includes, namespace, holder, prefix code, deduplicated trampoline
return typedefs) and class_cpp_register (the per-instantiation block); class_hpp
now declares every instantiation's register function; add struct_enum_register.
- class_writer.write() builds one file pair per class, deduplicating file-scope
trampoline typedefs (so a type shared by several instantiations is not
redefined) and naming files after the class (CppClassInfo.py_name_base()).
- module_writer emits one include per class; register calls are unchanged. It
warns if two classes would collide on the same wrapper file name.
This reduces the generated file count (and the translation units re-parsing the
heavy shared headers) without changing what is registered.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Regenerate with the combined-per-class wrappers: each class's template
instantiations now share one {Class}.cppwg.hpp/.cpp instead of one pair per
instantiation. Shapes drops from 19 to 15 wrapper files, cells from 34 to 22;
the module main files include one header per class and their register calls are
unchanged.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR implements issue #30 by consolidating per-template-instantiation wrapper outputs into a single wrapper file pair per class (one shared .cppwg.hpp/.cpp containing multiple register_* functions), reducing generated file count while preserving explicit instantiation registration.
Changes:
- Refactors class wrapper generation to emit one shared C++ preamble per class and append one registration block per instantiation.
- Updates module generation to include one wrapper header per class (using a new
py_name_base()), while still callingregister_*for each instantiation. - Adjusts templates and tests to match the new “shared header + per-instantiation register block” structure.
Reviewed changes
Copilot reviewed 53 out of 53 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_class_writer.py | Updates writer-facing tests for shared cpp preamble and per-instantiation register blocks. |
| examples/shapes/wrapper/primitives/Shape.cppwg.hpp | Consolidates multiple Shape instantiation declarations into one header. |
| examples/shapes/wrapper/primitives/Shape.cppwg.cpp | Consolidates multiple Shape instantiation definitions into one cpp. |
| examples/shapes/wrapper/primitives/Shape_3.cppwg.hpp | Removes per-instantiation header in favor of consolidated header. |
| examples/shapes/wrapper/primitives/Shape_3.cppwg.cpp | Removes per-instantiation cpp in favor of consolidated cpp. |
| examples/shapes/wrapper/primitives/Rectangle.cppwg.cpp | Moves typedef into per-class register block layout (generator consistency). |
| examples/shapes/wrapper/primitives/Cuboid.cppwg.cpp | Moves typedef into per-class register block layout (generator consistency). |
| examples/shapes/wrapper/primitives/_pyshapes_primitives.main.cppwg.cpp | Updates module includes to consolidated Shape wrapper header. |
| examples/shapes/wrapper/geometry/Point.cppwg.hpp | Consolidates multiple Point instantiation declarations into one header. |
| examples/shapes/wrapper/geometry/Point.cppwg.cpp | Consolidates multiple Point instantiation definitions into one cpp. |
| examples/shapes/wrapper/geometry/Point_3.cppwg.hpp | Removes per-instantiation header in favor of consolidated header. |
| examples/shapes/wrapper/geometry/Point_3.cppwg.cpp | Removes per-instantiation cpp in favor of consolidated cpp. |
| examples/shapes/wrapper/geometry/_pyshapes_geometry.main.cppwg.cpp | Updates module includes to consolidated Point wrapper header. |
| examples/shapes/wrapper/composites/Square.cppwg.cpp | Moves typedef into per-class register block layout (generator consistency). |
| examples/cells/dynamic/wrappers/all/Scene.cppwg.hpp | Consolidates multiple Scene instantiation declarations into one header. |
| examples/cells/dynamic/wrappers/all/Scene.cppwg.cpp | Consolidates multiple Scene instantiation definitions into one cpp. |
| examples/cells/dynamic/wrappers/all/Scene_3.cppwg.hpp | Removes per-instantiation header in favor of consolidated header. |
| examples/cells/dynamic/wrappers/all/Scene_3.cppwg.cpp | Removes per-instantiation cpp in favor of consolidated cpp. |
| examples/cells/dynamic/wrappers/all/PottsMesh.cppwg.hpp | Adds consolidated PottsMesh wrapper header declaring both instantiations. |
| examples/cells/dynamic/wrappers/all/PottsMesh.cppwg.cpp | Consolidates PottsMesh instantiation definitions into one cpp. |
| examples/cells/dynamic/wrappers/all/PottsMesh_3.cppwg.hpp | Removes per-instantiation header in favor of consolidated header. |
| examples/cells/dynamic/wrappers/all/PottsMesh_3.cppwg.cpp | Removes per-instantiation cpp in favor of consolidated cpp. |
| examples/cells/dynamic/wrappers/all/PottsMesh_2.cppwg.hpp | Removes per-instantiation header in favor of consolidated header. |
| examples/cells/dynamic/wrappers/all/PetscUtils.cppwg.cpp | Moves typedef into per-class register block layout (generator consistency). |
| examples/cells/dynamic/wrappers/all/Node.cppwg.hpp | Consolidates multiple Node instantiation declarations into one header. |
| examples/cells/dynamic/wrappers/all/Node.cppwg.cpp | Consolidates multiple Node instantiation definitions into one cpp. |
| examples/cells/dynamic/wrappers/all/Node_3.cppwg.hpp | Removes per-instantiation header in favor of consolidated header. |
| examples/cells/dynamic/wrappers/all/Node_3.cppwg.cpp | Removes per-instantiation cpp in favor of consolidated cpp. |
| examples/cells/dynamic/wrappers/all/MeshFactory.cppwg.hpp | Adds consolidated MeshFactory wrapper header declaring both instantiations. |
| examples/cells/dynamic/wrappers/all/MeshFactory.cppwg.cpp | Consolidates MeshFactory instantiation definitions into one cpp. |
| examples/cells/dynamic/wrappers/all/MeshFactory_PottsMesh_3.cppwg.hpp | Removes per-instantiation header in favor of consolidated header. |
| examples/cells/dynamic/wrappers/all/MeshFactory_PottsMesh_3.cppwg.cpp | Removes per-instantiation cpp in favor of consolidated cpp. |
| examples/cells/dynamic/wrappers/all/MeshFactory_PottsMesh_2.cppwg.hpp | Removes per-instantiation header in favor of consolidated header. |
| examples/cells/dynamic/wrappers/all/MacroMesh.cppwg.hpp | Adds consolidated MacroMesh wrapper header declaring both instantiations. |
| examples/cells/dynamic/wrappers/all/MacroMesh.cppwg.cpp | Consolidates MacroMesh instantiation definitions into one cpp. |
| examples/cells/dynamic/wrappers/all/MacroMesh_3_3.cppwg.hpp | Removes per-instantiation header in favor of consolidated header. |
| examples/cells/dynamic/wrappers/all/MacroMesh_3_3.cppwg.cpp | Removes per-instantiation cpp in favor of consolidated cpp. |
| examples/cells/dynamic/wrappers/all/MacroMesh_2_2.cppwg.hpp | Removes per-instantiation header in favor of consolidated header. |
| examples/cells/dynamic/wrappers/all/Facet.cppwg.hpp | Renames guard/header naming to consolidated per-class naming. |
| examples/cells/dynamic/wrappers/all/Facet.cppwg.cpp | Updates includes/typedef placement for consolidated per-class naming. |
| examples/cells/dynamic/wrappers/all/Corner.cppwg.hpp | Renames guard/header naming to consolidated per-class naming. |
| examples/cells/dynamic/wrappers/all/Corner.cppwg.cpp | Updates includes/typedef placement for consolidated per-class naming. |
| examples/cells/dynamic/wrappers/all/Cell.cppwg.cpp | Moves typedef into per-class register block layout (generator consistency). |
| examples/cells/dynamic/wrappers/all/AbstractMesh.cppwg.hpp | Adds consolidated AbstractMesh wrapper header declaring both instantiations. |
| examples/cells/dynamic/wrappers/all/AbstractMesh.cppwg.cpp | Consolidates AbstractMesh instantiation definitions into one cpp. |
| examples/cells/dynamic/wrappers/all/AbstractMesh_3_3.cppwg.hpp | Removes per-instantiation header in favor of consolidated header. |
| examples/cells/dynamic/wrappers/all/AbstractMesh_3_3.cppwg.cpp | Removes per-instantiation cpp in favor of consolidated cpp. |
| examples/cells/dynamic/wrappers/all/AbstractMesh_2_2.cppwg.hpp | Removes per-instantiation header in favor of consolidated header. |
| examples/cells/dynamic/wrappers/all/_pycells_all.main.cppwg.cpp | Updates module includes to consolidated per-class wrapper headers. |
| cppwg/writers/module_writer.py | Switches module wrapper includes to per-class headers; adds collision detection. |
| cppwg/writers/class_writer.py | Implements shared cpp header + per-instantiation register blocks; emits single file pair per class. |
| cppwg/templates/pybind11_default.py | Splits templates into cpp preamble vs register blocks; updates hpp to declare all instantiations. |
| cppwg/info/class_info.py | Adds py_name_base() and uses it for consistent base naming across instantiations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…r file Add a README tip noting the combined-per-class output: Foo<2> and Foo<3> are wrapped in one Foo.cppwg.hpp/.cpp pair (with a register_Foo_2_class and register_Foo_3_class each) rather than a file pair per instantiation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The combined-per-class write() loop iterates class_info.decls and indexes class_info.py_names by the same position, but the precondition only compared decls against cpp_names. A short py_names would then fail as an IndexError mid-loop (or silently mislabel a wrapper). Check all three parallel lists are equal length up-front and raise a message naming the class and the lengths, so a desync fails deterministically and is easy to diagnose. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Each class writes one wrapper file pair named after the class. If two classes in a module map to the same file stem, one overwrites the other's .hpp/.cpp and they collide on the register_..._class symbols, producing a broken output set that only surfaces later as a confusing compile/link error. There is no benign case, so raise a ValueError at generation time naming both classes and pointing at the fix, instead of logging a warning and continuing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
When several instantiations share one wrapper file, each instantiation's alias
typedef (typedef <cpp> <py>;) previously sat inside its own registration block,
after that block's generator pre-code. A custom generator whose pre-code refers
to the class by its alias - e.g. pychaste's CellsGenerator, which emits an
override class inheriting the alias - then used the alias before it was declared,
so the combined wrapper failed to compile (the shapes/cells examples do not
exercise that path, so it slipped through).
Gather every instantiation's alias typedef and emit them once in the shared
class_cpp_header preamble (${class_typedefs}) ahead of all registration code, so
each block's generator pre-code, trampoline override class and registration can
refer to any instantiation by its alias regardless of order. Drop the per-block
typedef from class_cpp_register and struct_enum_register.
Regenerate the shapes and cells example wrappers to the new layout. Verified:
157 unit tests pass; shapes 6/6 and cells 8/8 example tests pass; the full
pychaste combined wrapper set (273 files, incl. CellsGenerator and the
AbstractMesh trampoline) compiles, links and imports.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The struct-enum registration template concatenated ${generator_pre_code}
directly with the void register_... line. A custom generator's
get_class_cpp_pre_code is not guaranteed to end with a newline, so a
non-empty pre-code could emit invalid C++ (e.g. class X{};void register...).
Insert an explicit newline, matching class_cpp_register.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The custom-generator API (cppwg.templates.custom.Custom) returns raw C++ with no trailing-newline guarantee. Three template slots splice that output directly ahead of another line, so a missing newline can produce invalid C++: module_pre_code precedes the class #include lines (a directive that no longer starts a line), module_code precedes the module's closing brace, and generator_def_code precedes the .def() chain terminator (a trailing // comment would swallow it). Add ensure_trailing_newline() and apply it at the three consumption points. Normalising only non-empty output leaves generator-free files (all the examples) byte-identical, unlike a template-level newline. Also add get_module_pre_code() to the Custom base class: module_writer already calls it, so a generator subclassing Custom without it would raise AttributeError. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Restore the closing triple-quote on ensure_trailing_newline's docstring; a stray edit had dropped it, turning the rest of utils.py into one unterminated string literal (E999 / black parse failure). Pin black's target-version to the project's minimum supported Python (py310, matching requires-python) instead of listing every supported version; black gates formatting on the oldest target, so this is equivalent output with less to maintain. Reformat two tuple-unpack assignments accordingly. 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.
Supports #30