NWChemEx / NWChemEx/PluginPlay
Remove shared_ptr from Python API
Nobody has claimed this yet.
- Dominant language
- C++
- Stars
- 13
- Forks
- 1
- Avg merge
- 43m
- Merged PRs (30d)
- 4
Description
AFAIK the only way to add a Python module to PluginPlay is via a pattern like:
# Redundant import is issue
from pluginplay import pluginplay
from cppyy.gbl.std import make_shared
class MyModule(pluginplay.ModuleBase):
def __init__(self):
super().__init__(self)
# rest of ctor
def run_(self, inputs, submods):
# rest of body
mm = pluginplay.ModuleManager()
mm.add_module("module key", make_shared[MyModule]())
# Ideal solution
# (currently doesn't work see #269)
# mm.add_module("module key", MyModule())
#Also fine
#(currently doesn't work leads to double free or corruption error)
# mm.add_module[MyModule]("module key")
# Alternatively could expose a Python free function to wrap
# calling shared_ptr from Python, e.g.
#pluginplay.add_module(mm, MyModule)
#
Relying on std::shared_ptr is very non-Pythonic and I would suspect users would prefer one of the three suggestions over dealing with std::shared_ptr
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start by reviewing the Python binding entry points for ModuleBase and ModuleManager.add_module, then read the referenced issue #269 and compare the three proposed calling patterns. Done means a Python module can be registered without users explicitly constructing std::shared_ptr, while avoiding the reported double-free or corruption failure.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp, python
- Domain
- api, developer-experience
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 28/100