Skip to content

Use prefix increment in make_iterator#247

Merged
wjakob merged 2 commits into
pybind:masterfrom
aldanor:iterators
Jun 21, 2016
Merged

Use prefix increment in make_iterator#247
wjakob merged 2 commits into
pybind:masterfrom
aldanor:iterators

Conversation

@aldanor

@aldanor aldanor commented Jun 17, 2016

Copy link
Copy Markdown
Member

This patch switches to using prefix increment for iterators returned by make_iterator. This is generally a good idea since postfix increment involves creating a temporary object which may sometimes be expensive.

Btw I've just noticed there's been another related PR (#183) which seems to have been abandoned.

@wjakob

wjakob commented Jun 21, 2016

Copy link
Copy Markdown
Member

Both implementations (pre and postfix) will make a copy of the object (rather than returning a temporary), since the return type of the lambda function is ValueType. When the iterable stores pointers, this means a copy of the pointer (i.e. no costly copy of the pointee).

(So it's not clear to me how this is different/better than the prior approach -- can you clarify?)

@aldanor

aldanor commented Jun 21, 2016

Copy link
Copy Markdown
Member Author

@wjakob

  1. The assumption that copying pointee is always cheaper than pointing the iterator is not always true. I'm currently working on a codebase where the yielded value type is simple (like an int), but the iterator itself contains a shared_ptr around a handle that wraps a C-level FFI pointer. This way, copying the iterator is more costly and is completely uncalled for.
  2. pybind11 is the first library I've encountered (STL, Streams, cppitertools, a bunch of others) that requires implementing postfix++, anywhere else (including language constructs like range for, etc) just implementing a prefix increment is entirely sufficient.

@wjakob

wjakob commented Jun 21, 2016

Copy link
Copy Markdown
Member

ok -- I think those are good reasons. Thanks!

@wjakob
wjakob merged commit 407c292 into pybind:master Jun 21, 2016
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