Correctness fix (pull #578) plus efficiency improvements #579
Conversation
- Fix 'before' return on eof/timeout/error. - Fix behavior when searchwindowsize is increased between calls to expect_loop (existing code could discard required content). Note, this is less efficient, but it is correct. The prior changes improved benchmarks but broke correctness; I haven't benchmarked to see if this is better or worse than the original code, before buffer_type use was introduced. It would be possible to make this more efficient with further complexity. Breakage introduced: pexpect@fd7332f#diff-244f4b51bfbbbcb072d1428a79b9f4f7 Prior partial fix: pexpect@5a2e63f#diff-244f4b51bfbbbcb072d1428a79b9f4f7
|
Note, obsoletes (includes) #578 |
|
Primary reasons for this change: The prior changes lost the meaning of seachwindowsize and introduced numerous copies of the entire buffer into some usage patterns. This change substantially reduces copies. |
Merge from master
Merge from master
|
This looks fine but I'd prefer if the coverage didn't drop as much for this fix. |
|
What is it going to take to get this merged? If I wrote a test which fails with the old code and succeeds with the new, would that be sufficient? This has been pending a long time, and I'd really like to get it submitted. |
There was a problem hiding this comment.
Sorry this has lingered so long. Yes, I think it would be a good idea to add a test - besides anything else, it will ensure that we don't reintroduce the bug you're fixing! It might also help me to understand what it is you're fixing - I think I can see it, but it would be nice to make it explicit.
Merge updates from master
Merge updates from master
Merge from master
expect_{loop,async} call new_data at start, when there isn't really new data. Reviewer requested a split of functionality here into two routines.
|
Requested refactoring done, three regression tests added. Please re-review, thanks! |
Pull request combines the correctness fix in #578 and efficiency improvements to stop copying large amounts of data in numerous codepaths (and generally reduce data copies).