refactor: remove Py_Get_ID and cached string objects
#263
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #263 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 14 14
Lines 1416 1416
Branches 175 175
=========================================
Hits 1416 1416 Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Pull request overview
This PR refactors the codebase to remove the custom Py_Get_ID macro system and cached string objects, replacing them with direct string literals. This simplifies the code by eliminating the need for string object caching and macro-based string handling.
- Removed all
Py_Declare_IDandPy_Get_IDmacro definitions frominclude/optree/pymacros.h - Replaced
Py_Get_ID()calls with direct string literals in pybind11 functions (py::getattr,py::setattr) and C API functions (PyObject_GetAttrString) - Updated all affected files to use the simplified string handling approach
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| include/optree/pymacros.h | Removed all Py_Declare_ID and Py_Get_ID macro definitions and cached string declarations |
| src/optree.cpp | Replaced Py_Get_ID with py::str() for module attribute setting |
| include/optree/pytypes.h | Replaced Py_Get_ID with PyObject_GetAttrString for C API calls and direct strings for py::getattr calls |
| src/treespec/treespec.cpp | Replaced Py_Get_ID(copy) with direct string "copy" in py::getattr calls |
| src/treespec/serialization.cpp | Replaced Py_Get_ID with direct strings for __name__, __module__, __qualname__ attribute access |
| src/treespec/flatten.cpp | Replaced Py_Get_ID with direct strings for copy, default_factory, and maxlen attribute access |
| src/treespec/constructors.cpp | Replaced Py_Get_ID with direct strings for copy, default_factory, and maxlen attribute access |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
1 participant
Description
Describe your changes in detail.
Motivation and Context
Why is this change required? What problem does it solve?
If it fixes an open issue, please link to the issue here.
You can use the syntax
close #15213if this solves the issue #15213Types of changes
What types of changes does your code introduce? Put an
xin all the boxes that apply:Checklist
Go over all the following points, and put an
xin all the boxes that apply.If you are unsure about any of these, don't hesitate to ask. We are here to help!
make format. (required)make lint. (required)make testpass. (required)