Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reuse PyEnumerable, removing need for PyKeyValuePairEnumerable #204

Merged
merged 2 commits into from
Sep 17, 2024

Conversation

atifaziz
Copy link
Contributor

@atifaziz atifaziz commented Sep 17, 2024

PyKeyValuePairEnumerable<,> had almost an identical implementation to PyEnumerable<> except for the conversion of the PyObject value. The former calls PyObject.As<,>:

using PyObject pyObject = PyObject.Create(result);
current = pyObject.As<TKey, TValue>();

whereas the latter calls PyObject.As<>:

using PyObject pyObject = PyObject.Create(result);
current = pyObject.As<TValue>();

That's a lot of duplication to maintain, sync, test and cover. This PR consolidates the duplication so there's only one bulk implementation. It does this by adding another type parameter (called TImporter) to PyEnumerable<> that determines how the PyObject is imported (into presumably a managed type):

internal class PyEnumerable<TValue, TImporter> : IEnumerable<TValue>, IEnumerator<TValue>, IDisposable
    where TImporter : IPyObjectImporter<TValue>
{
    // omitted for brevity
}

The importer has a static member, which bears zero cost (that is, no allocation and no virtual dispatch needed):

interface IPyObjectImporter<out T>
{
    static abstract T Import(PyObject pyObj);
}

The two import strategies, PyObject.As<> and PyObject.As<,>, are then embodied into implementations of this interface. The import strategy is chosen at the time PyEnumerable<,> is instantiated. A PyEnumerable<> with a single type parameter still exists, but which uses the default strategy of PyObject.As<> so most of the code is untouched except in PyDictionary<,>.GetEnumerator.

@tonybaloney
Copy link
Owner

Love it! this is a lot cleaner

@tonybaloney tonybaloney merged commit 6379c99 into tonybaloney:main Sep 17, 2024
24 of 26 checks passed
@atifaziz atifaziz deleted the rm/PyKeyValuePairEnumerable branch September 18, 2024 06:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants