LiveEdit: calculate a real script difference

LiveEdit: calculate a real script difference

[Peter Rybin, 2010-04-15 18:59:24]

Hi Soren

Here I am adding general-purpose diff algorithm and its specialization for
script line-by-line comparison.

C++ tests check general-purpose API, JS tests check string line-by-line
specialization.

Peter

[Søren Thygesen Gjesse, 2010-04-16 07:01:44]

LGTM

http://codereview.chromium.org/1652008/diff/3001/4004
File src/liveedit.cc (right):

http://codereview.chromium.org/1652008/diff/3001/4004#newcode46
src/liveedit.cc:46: // A simple implementation of dynamic programming algorithm.
I solves a problem
a problem -> the problem

http://codereview.chromium.org/1652008/diff/3001/4004#newcode47
src/liveedit.cc:47: // of finding a difference of 2 arrays. It uses a table of
results
a difference -> the difference

http://codereview.chromium.org/1652008/diff/3001/4004#newcode48
src/liveedit.cc:48: // for subproblems. Each cell contains a number together
with 2-bit flag
for -> of?

http://codereview.chromium.org/1652008/diff/3001/4004#newcode49
src/liveedit.cc:49: // that helps building chunk list.
the chunk list.

http://codereview.chromium.org/1652008/diff/3001/4004#newcode68
src/liveedit.cc:68: // in table together with flags showing a path through
subproblems.
in table -> in the table

http://codereview.chromium.org/1652008/diff/3001/4004#newcode124
src/liveedit.cc:124: SKIP_ANY
Maybe add MAX_DIRECTION_FLAG = SKIP_ANY and assert MAX_DIRECTION_FLAG < (1 <<
kDirectionSizeBits) somewhere.

(maybe MAX_DIRECTION_FLAG is not the best name).

http://codereview.chromium.org/1652008/diff/3001/4004#newcode128
src/liveedit.cc:128: // All results numbers are multiplied by 4 to have a space
for 2-bit flag.
results -> result

Maybe leave out the actual constant values from the comment e.g. "All results
are shifted to make space for flags in the lower bits"

http://codereview.chromium.org/1652008/diff/3001/4004#newcode263
src/liveedit.cc:263: 
Two empty lines.

http://codereview.chromium.org/1652008/diff/3001/4004#newcode303
src/liveedit.cc:303: 
Ditto.

http://codereview.chromium.org/1652008/diff/3001/4004#newcode336
src/liveedit.cc:336: 
Ditto.

http://codereview.chromium.org/1652008/diff/3001/4005
File src/liveedit.h (right):

http://codereview.chromium.org/1652008/diff/3001/4005#newcode122
src/liveedit.h:122: public:
No newline after public.

http://codereview.chromium.org/1652008/diff/3001/4005#newcode139
src/liveedit.h:139: // Puts another chunk in result list. Note that technically
speaking
technically -> strictly

http://codereview.chromium.org/1652008/diff/3001/4009
File test/cctest/test-liveedit.cc (right):

http://codereview.chromium.org/1652008/diff/3001/4009#newcode59
test/cctest/test-liveedit.cc:59: 
Two empty lines (and some more places below).

http://codereview.chromium.org/1652008/diff/3001/4010
File test/mjsunit/debug-liveedit-diff.js (right):

http://codereview.chromium.org/1652008/diff/3001/4010#newcode33
test/mjsunit/debug-liveedit-diff.js:33: function CheckCompareOneWay(s1, s2) {
Maybe add a third (optional) argument for the expected text formatted diff (you
are calculating it anyway) and check that as well.

[Peter Rybin, 2010-04-16 16:30:05]

http://codereview.chromium.org/1652008/diff/3001/4004
File src/liveedit.cc (right):

http://codereview.chromium.org/1652008/diff/3001/4004#newcode46
src/liveedit.cc:46: // A simple implementation of dynamic programming algorithm.
I solves a problem
On 2010/04/16 07:01:44, Søren Gjesse wrote:
> a problem -> the problem

Done.

http://codereview.chromium.org/1652008/diff/3001/4004#newcode47
src/liveedit.cc:47: // of finding a difference of 2 arrays. It uses a table of
results
On 2010/04/16 07:01:44, Søren Gjesse wrote:
> a difference -> the difference

Done.

http://codereview.chromium.org/1652008/diff/3001/4004#newcode48
src/liveedit.cc:48: // for subproblems. Each cell contains a number together
with 2-bit flag
On 2010/04/16 07:01:44, Søren Gjesse wrote:
> for -> of?

Done.

http://codereview.chromium.org/1652008/diff/3001/4004#newcode49
src/liveedit.cc:49: // that helps building chunk list.
On 2010/04/16 07:01:44, Søren Gjesse wrote:
> the chunk list.

Done.

http://codereview.chromium.org/1652008/diff/3001/4004#newcode68
src/liveedit.cc:68: // in table together with flags showing a path through
subproblems.
On 2010/04/16 07:01:44, Søren Gjesse wrote:
> in table -> in the table

Done.

http://codereview.chromium.org/1652008/diff/3001/4004#newcode124
src/liveedit.cc:124: SKIP_ANY
On 2010/04/16 07:01:44, Søren Gjesse wrote:
> Maybe add MAX_DIRECTION_FLAG = SKIP_ANY and assert MAX_DIRECTION_FLAG < (1 <<
> kDirectionSizeBits) somewhere.
> 
> (maybe MAX_DIRECTION_FLAG is not the best name).

Done.

http://codereview.chromium.org/1652008/diff/3001/4004#newcode128
src/liveedit.cc:128: // All results numbers are multiplied by 4 to have a space
for 2-bit flag.
On 2010/04/16 07:01:44, Søren Gjesse wrote:
> results -> result
> 
> Maybe leave out the actual constant values from the comment e.g. "All results
> are shifted to make space for flags in the lower bits"

Done.

http://codereview.chromium.org/1652008/diff/3001/4004#newcode263
src/liveedit.cc:263: 
On 2010/04/16 07:01:44, Søren Gjesse wrote:
> Two empty lines.

Done.

http://codereview.chromium.org/1652008/diff/3001/4004#newcode303
src/liveedit.cc:303: 
On 2010/04/16 07:01:44, Søren Gjesse wrote:
> Ditto.

Done.

http://codereview.chromium.org/1652008/diff/3001/4004#newcode336
src/liveedit.cc:336: 
On 2010/04/16 07:01:44, Søren Gjesse wrote:
> Ditto.

Done.

http://codereview.chromium.org/1652008/diff/3001/4009
File test/cctest/test-liveedit.cc (right):

http://codereview.chromium.org/1652008/diff/3001/4009#newcode59
test/cctest/test-liveedit.cc:59: 
On 2010/04/16 07:01:44, Søren Gjesse wrote:
> Two empty lines (and some more places below).

Done.

http://codereview.chromium.org/1652008/diff/3001/4010
File test/mjsunit/debug-liveedit-diff.js (right):

http://codereview.chromium.org/1652008/diff/3001/4010#newcode33
test/mjsunit/debug-liveedit-diff.js:33: function CheckCompareOneWay(s1, s2) {
On 2010/04/16 07:01:44, Søren Gjesse wrote:
> Maybe add a third (optional) argument for the expected text formatted diff
(you
> are calculating it anyway) and check that as well.

This is a good idea, but I'd rather not over-complicate tests for now. The
quality of diff (how optimal it is) I am checking in C++ tests. Here I only
check that result is correct.

However it can be added it later.