Fix bug in cpp iteration over rvalue-dependant attribute #3828
Conversation
|
Should I open an issue ? Should I copy the commit message to the PR description ? |
Definitely copy the commit message here so that the PR makes sense. I'm not convinced that an issue is really needed but others might disagree. |
OK, I updated the PR description with the description in the commit message. |
The implementation of the Python-style `for ... in` loop over an
iterable cpp sequence (using `begin()` and `end()`) needs to determine
whether the sequence should be stored in a temporary variable or
if it can be iterated over directly.
The previous code assumed that attributes are always safe to be
iterated directly, but attributes can depend on an rvalue, as in:
for i in returns_object_with_vector_attribute().vector:
...
In such a case we need to store the `in` operand in a temporary value
on which `begin()` and `end()` can be safely called.
It might make sense to move the test into tests/run/cpp_iterators
|
I noticed that the test builds on OSX seemed to fail because I used C++11-style list-initialization in the external C++ code to support my testcase, so I modified it. |
The implementation of the Python-style `for ... in` loop over an iterable cpp sequence (using begin() and end()) needs to determine whether the sequence should be stored in a temporary variable or if it can be iterated over directly. A previous fix considers that the sequence can be accessed directly if `sequence.is_simple()` evaluates to True. However it is at least theoretically be possible for an expression to both be simple and to depend on temporary subexpressions. In that case, because the CppIteratorNode is always a temporary, the compiler will by default eagerly free the temporary subexpressions for reuse after the result of the sequence has been evaluated the first time. This will result in subsequent calls to `result()` returning `None` even though the required variables are still available and in scope in the generated C++ code. This commit postpones the release of temporary subexpressions until the CppIteratorNode is itself disposed-of when the sequence is accessed directly.
|
Just closing and re-opening to re-run the tests (just trying to see if I can clear up some old inactive PRs) |
|
I've moved the tests into the main cpp_iterators file (but made no other changes to them). I think I'm happy with this PR and would like to merge it. I'll wait for a bit to see if there's other comments/objections though |
|
I'd been planning to merge this after the next release - I think it's a worthwhile (if slightly obscure) bug fix (link back to #4022) |
|
Thanks, and sorry it took a while to merge |
|
Thanks for merging @da-woods :) and sorry in turn I didn't answer sooner. |
The implementation of the Python-style
for ... inloop over an iterable cpp sequence (usingbegin()andend()) needs to determine whether the sequence should be stored in a temporary variable or if it can be iterated over directly.The previous code assumed that attributes are always safe to be iterated directly, but attributes can depend on an rvalue, as in:
In such a case we need to store the
inoperand in a temporary value on whichbegin()andend()can be safely called.It might make sense to move the test into tests/run/cpp_iterators