Comment 14 for bug 1835968

Revision history for this message
Bryce Harrington (bryce) wrote :

I don't know Ruby very well, but I've narrowed it down to a specific line within the patch. The original code had:

    if @sync or @wbuffer.size > BLOCK_SIZE or idx = @wbuffer.rindex("\n")

The patch in question drops the final conditional:

    if @sync or @wbuffer.size > BLOCK_SIZE

The rest of the patch replaces some code that had processed the buffer using the idx with different processing code that doesn't use idx, and I suspect the patch author dropped the condition since idx was no longer needed. However, .rindex("\n") has a side-effect of returning nil when "\n" is not found in the buffer. So in fact, this passes the testcase (but probably breaks lots of other stuff):

    if @sync or @wbuffer.size > BLOCK_SIZE or idx = nil

as does of course

    if @sync or @wbuffer.size > BLOCK_SIZE or nil

So, I think a suitable fix would be to partially restore the line to:

    if @sync or @wbuffer.size > BLOCK_SIZE or @wbuffer.rindex("\n")

This enables the testcase to pass without re-introducing the now-unnecessary idx variable, and leaves the remaining optimization work intact.