-
Notifications
You must be signed in to change notification settings - Fork 2.2k
pytypes: Add iterable_t<T> #2950
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
base: master
Are you sure you want to change the base?
Conversation
628ac3e
to
8837d7d
Compare
8837d7d
to
9ec5905
Compare
9ec5905
to
8273530
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool! Just questions regarding compatibility with the smart_holder branch.
Just commenting to say I'm working on something atm for which this would be useful :-) |
This needs to be resurrected after the sweeping changes for clang-format and Python 2 drop. You could transfer the diffs to a new PR and work on it there. |
It was luckily small enough w/ just a merge + conflict resolution. I think the review is still fine w/ merge. Let me know if you'd still like a new PR. |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
High-level:
At the moment list_caster in stl.h explicitly checks whether the input argument is a sequence, but I would like to argue that that's slightly too restrictive and iterable should be accepted.
The code seems to be dicy ("bad! For now,") and the gain seems really minute ("slightly too restrictive").
The current implementation doesn't seem exception safe (leaks b/o missed Py_DECREF if exceptions are thrown before the control flow reaches those).
But really I feel it's best not to get into this scraping-the-bottom-of-the-barrel territory at all.
Aye - so you think it's best to not introduce the feature? Fine by me, just want to clarify some points.
Too restrictive in what sense? Not an iterator, or shouldn't check types, or something else? |
Sorry I wasn't clear, the "slightly too restrictive" quote is from here: #1773 I.e. my interpretation was that even the original requester was thinking the current behavior is only "slightly too restrictive". (I agree.) My cost/benefit assessment is just one vote! Yes, I'd say let's not add this, but if there are other opinions I'll reconsider. I'm just offering my current opinion. |
Sounds good! I'm not too wed to this, but will await opinions, and close if we don't hear back for a bit (~month?) @alexdewar If you're available, is this still useful as-is? |
Description
Resolves #1773 as an alternative / less disruptive approach
Drawing from RobotLocomotion/drake#14898
Suggested changelog entry: