• 
      

    Improve move detection in several scenarios.

    Review Request #4854 — Created Oct. 28, 2013 and submitted

    Information

    Review Board
    master

    Reviewers

    Improve move detection in several scenarios.

    Move detection's abilities were pretty limited, and there were many
    cases where it simply didn't even work. This change addresses several
    cases:

    1) Blank lines in a move region. Any blank lines (or whitespace-only
    lines) would interrupt the move region, causing new ones to form
    (assuming those individually were considered valid, useful move
    regions). Now blank lines will be folded into the existing region,
    if they bridge two regions.

    2) Adjacent but different move regions didn't work. Only the first
    would win. There would need to be at least one line between regions.
    Now any regions anywhere work, in any order.

    3) Only insert/delete lines were considered for moves, which was a
    problem when moving some functions that resulted in replace lines
    (such as when one function was replaced by another). Now replace
    lines are factored in. The left-hand side is treated as a delete,
    and the right-hand side is treated as an insert.

    4) The new move flag display assumed consecutive lines constituted the
    same move region, even if that wasn't true. It now shows a new
    beginning move flag for a region if it's a different region from
    the previous line.

    There's also some improvements to the display of the move flags. Move
    flags on replace lines caused some padding issues with line numbers that
    broke alignment, so that's been fixed.

    Along with this, 2 move flags on a replace line is a bit weird and
    broken-looking typically, so in an effort to get a step closer to
    something good, arrows are now shown pointing to the section moved,
    padding has been added between the flag and the text contents, and
    "line" has been added to the text.

    Tested several scenarios.

    I tested some of my existing move detection review requests and saw that the
    general case continues to work fine.

    I then tested the scenarios depicted by the attached screenshots.

    Blank lines

    Uploaded a change that moved a block of code, blank lines included.

    With the old code, this would appear as a series of individual move regions.

    With the new code, it appeared as one solid move region.

    Adjacent move regions

    Moved a region of lines from one place in a file to another, and then
    swapped half the lines with the other half.

    With the old code, it appeared as one move region, covering one half of
    that block. The code couldn't handle an adjacent move region, so that's
    where it stopped.

    With the new code, I got two move regions, one for each half. Checked
    the insert block and the remove block to make sure both ends indicated
    the proper source/destination lines.

    Replace lines

    Uploaded a change that:

    • Swapped two similar lines, generating replace lines
    • Moved a line to a new location, generating an insert, and replaced the
      original line with a similar line, generating a replace (instead of a
      remove).

    With the old code, there wasn't any move information at all, since it
    skipped replace lines.

    With the new code, I got move regions for each move. The replace line
    swap in particular is neat, though admittedly a bit weird. Some of the
    new style changes help a bit though.


    david
    1. Ship It!
    2. 
        
    chipx86
    Review request changed
    Status:
    Completed