Test recursive dispatch using visitor pattern. #3365
Conversation
dyershov
changed the title
|
@rwgk Tagging you in this PR as per #3357 (comment). Apologies for CI failing. I have to learn how to run checks locally so that I can fix things. |
dyershov
force-pushed
the
BUG-3357/recursive-dispatch-test
branch
3 times, most recently
from
56edf9b to
12eb9dc
Compare
|
Try |
dyershov
force-pushed
the
BUG-3357/recursive-dispatch-test
branch
from
9e23ee4 to
17cd089
Compare
|
@dyershov The failing test is a flake so this looks good. |
|
@Skylion007, @rwgk, and @henryiii , no need to worry about the response time. Thank you all for the great project! Without it a lot of what I do would be very difficult in not impossible to achieve. Looking forward for possible resolution on this issue. |
| # lambda is a workaround, which adds extra frame to the | ||
| # current CPython thread. Removing lambda reveals the bug | ||
| # [https://github.com/pybind/pybind11/issues/3357] | ||
| (lambda: visitor(Data(first.value + second.value)))() |
There was a problem hiding this comment.
Shouldn't the lambda have not been here, and the test marked with xfail? It's not a segfault, right?
There was a problem hiding this comment.
Maybe an additional test.
I think it's important to keep this test (with the extra lambda) indefinitely. We want to be sure it doesn't break, even after the bug is fixed.
There was a problem hiding this comment.
I also thought about two tests: one expected to fail, another with a workaround. I can create another PR.
There was a problem hiding this comment.
May be nice ... but at the moment it doesn't buy as anything tangible (the existing comments are very clear) and it'll be an organic minor part of the work fixing the bug. I'd just leave it for later.
There was a problem hiding this comment.
Sounds good. Though, it would be fun to try and learn more about xfail.
There was a problem hiding this comment.
Up to you! I'll vote to merge if you send the PR.
Description
Implemented unit test aims at replicating BUG #3357. The implementation defines abstract C++ class that performs an operation on abstract member Data class and sends the result to a visitor. A trampoline class is used to bind Adder python class. Additionally two algorithms that use previously defined abstractions to perform operation on two or three objects. These algorithms are tested using a concrete python implementation. The recursive dispatch of overloaded operator fails in the case of algorithm that uses three objects. When calling a visitor indirectly from lambda function inside overloaded operation, the expected functionality is restored.
Suggested changelog entry: