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: Closed (submitted)

Loading...