Skip to content

Expose enum_ entries as new "__members__" attribute#666

Merged
jagerman merged 1 commit into
pybind:masterfrom
mdcb:master
Mar 3, 2017
Merged

Expose enum_ entries as new "__members__" attribute#666
jagerman merged 1 commit into
pybind:masterfrom
mdcb:master

Conversation

@mdcb

@mdcb mdcb commented Feb 13, 2017

Copy link
Copy Markdown
Contributor

Brings convenience of being able to list the entries making an Enum. Side effect simplifies the .export_values() implementation.

@jagerman

Copy link
Copy Markdown
Member

I didn't look at this in great detail, but a couple comments:

  • I wonder if __members__ would be a more appropriate choice for the name?

  • Judging from the travis output, there appears to be an issue with your code under Python 2.7. (You can ignore the PYTHON=3.5 GCC=6/7 failures, they are known issues).

@mdcb mdcb changed the title Expose enum_ entries as new "entries" attribute Expose enum_ entries as new "__items__" attribute Feb 14, 2017
@mdcb

mdcb commented Feb 14, 2017

Copy link
Copy Markdown
Contributor Author

I eventually chose '__items__'
Update should fix the 2.7 issue.

Comment thread include/pybind11/pybind11.h Outdated
void>::value;

auto entries = new std::unordered_map<Scalar, const char *>();
auto entries = new pybind11::dict();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use of new here is problematic (this will never be cleaned up.) Better to use standard python objects and rely on reference counting.

Comment thread include/pybind11/pybind11.h Outdated

/// Export enumeration entries into the parent scope
enum_ &export_values() {
#if !defined(PYPY_VERSION)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

neat! That's a nice simplification.

@wjakob

wjakob commented Feb 26, 2017

Copy link
Copy Markdown
Member

^@aldanor

Generally switching from the C++ map to dict is nice. With regards to the precise conventions (e.g. __items__, __members__, etc. Or perhaps just a type implementing the iterator interface?), I am wondering: is there a pythonic answer? Potentially related: #530.

@llchan

llchan commented Feb 27, 2017

Copy link
Copy Markdown

I'm a proponent of keeping it as close to the PEP 435 enums as possible, i.e. with __members__ and an iterator interface. Technically it's supposed to be in definition order too, how do we feel about using an OrderedDict for it? If we dont support <2.7 it should be pretty straightforward.

@wjakob

wjakob commented Feb 27, 2017

Copy link
Copy Markdown
Member

We always support 2.7 :)

@mdcb mdcb changed the title Expose enum_ entries as new "__items__" attribute Expose enum_ entries as new "__members__" attribute Feb 27, 2017
@mdcb

mdcb commented Feb 27, 2017

Copy link
Copy Markdown
Contributor Author

Lastest push fixes conflict issue and use __members__.
Trivial replacement of new pointer with pybind11:dict m_entries

        def("__repr__", [name, this](Type value) -> std::string {
            for (const auto &kv : this->m_entries) {
                if (kv.second.cast<Type>() == value)
                    return std::string(name) + "." + kv.first.cast<std::string>();
            }
            return std::string(name) + ".???";
        });

gives:

error: expected primary-expression before ‘>’ token
                 if (kv.second.cast<Type>() == value)

Looking into that and the iterator aspects. Anyone wishing to jump in welcomed.

@llchan

llchan commented Feb 27, 2017

Copy link
Copy Markdown

@wjakob Maybe my "<2.7" notation wasnt very clear. I just mean that if we don't care about supporting 2.6 and below, we can assume OrderedDict is available and not worry about fallback mechanisms.

@mdcb maybe you need kv.second.template cast<Type>()?

@llchan

llchan commented Feb 27, 2017

Copy link
Copy Markdown

Btw, the __iter__ in the CPython/enum34 enums do this:

def __iter__(cls):
    return (cls._member_map_[name] for name in cls._member_names_)

We could probably do the equivalent with m_entries.

Also I think we should follow their lead and make __members__ a property that returns a copy of m_entries, otherwise the user can mutate that dict.

@property
def __members__(cls):
    return cls._member_map_.copy()

@mdcb

mdcb commented Mar 1, 2017

Copy link
Copy Markdown
Contributor Author

Latest push should address the 'new' issue, readonly property, copy on access. Looked briefly at the iterator protocol - that might have to come from someone else and/or different PR.
PS: waiting for CI results to squash the 2 commits.

@jagerman

jagerman commented Mar 2, 2017

Copy link
Copy Markdown
Member

travis-ci is having some technical problems tonight, so you'll probably have to wait a bit to get a successful build.

Comment thread include/pybind11/pybind11.h Outdated
return std::string(name) + "." + kv.first.PREFIX_MEMBER_TEMPLATE cast<std::string>();
}
return std::string(name) + ".???";
#undef PREFIX_MEMBER_TEMPLATE

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can simply this whole block (and get rid of the ugly MSVC workaround macro) as:

        def("__repr__", [name, m_entries_ptr](Type value) -> str {
            for (const auto &kv : reinterpret_borrow<dict>(m_entries_ptr)) {
                if (pybind11::cast<Type>(kv.second) == value)
                    return str("{}.{}").format(name, kv.first);
            }
            return str("{}.???").format(name);
        });

@jagerman

jagerman commented Mar 3, 2017

Copy link
Copy Markdown
Member

Re: iterator interface: I don't think it is worthwhile. The main issue is that the iterator interface has to be added on the metatype, not the type itself (because iteration is on EnumType, not EnumType()). To support that, we'd have to stop inheriting from py::class_ and instead add a new metatype for enums (which is what Python itself does). That's a whole lot of work for very little gain. Better, I think, to accept this as is, and eventually just use Python's enum metatype (i.e. as proposed in #530) on Python 3.4+, with the current class_ wrapper providing an incomplete API fallback for < 3.4.

Aside from the __repr__ simplification I commented on, I think this is good to go.

@llchan

llchan commented Mar 3, 2017

Copy link
Copy Markdown

Oh derp, you're totally right, I just Ctrl-F'd the __iter__ without looking at what (meta)class it's in. The __members__ field is sufficient to at least extract that information, so I agree we can table the iterator interface for now.

@mdcb

mdcb commented Mar 3, 2017

Copy link
Copy Markdown
Contributor Author

@jagerman - squashed your __repr__ in lastest push

@jagerman

jagerman commented Mar 3, 2017

Copy link
Copy Markdown
Member

Assuming the WIP build succeeds, no need to squash (I can do that when merging).

@mdcb

mdcb commented Mar 3, 2017

Copy link
Copy Markdown
Contributor Author

Ok - note ther typo in docs.

@jagerman

jagerman commented Mar 3, 2017

Copy link
Copy Markdown
Member

Ah, well that needs a commit to fix it.

@mdcb

mdcb commented Mar 3, 2017

Copy link
Copy Markdown
Contributor Author

shake and bake

@jagerman jagerman merged commit 414ee16 into pybind:master Mar 3, 2017
@jagerman

jagerman commented Mar 3, 2017

Copy link
Copy Markdown
Member

Merged, thanks!

return m;
}, return_value_policy::copy);
def("__init__", [](Type& value, Scalar i) { value = (Type)i; });
def("__init__", [](Type& value, Scalar i) { new (&value) Type((Type) i); });

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unrelated to this PR but highlighted by the diff, lines 1145 / 1146 look odd.

@jagerman

jagerman commented Mar 4, 2017

Copy link
Copy Markdown
Member

Can you clarify "odd"?

@jagerman

jagerman commented Mar 4, 2017

Copy link
Copy Markdown
Member

Oh, you mean two identical constructors declared. Yes, that does look strange.

@jagerman jagerman modified the milestone: v2.1 Mar 19, 2017
@jagerman jagerman mentioned this pull request Apr 6, 2017
8 tasks
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.

4 participants