Code review comment for lp:~mhr3/libunity/diff-models

Revision history for this message
Paweł Stołowski (stolowski) wrote :

This MP is a great improvement and seems to be working fine (and also unit tests prove it), but it's really hard to understand and judge the code...
Could you please at very least add docs to the main data structures used in the algorithm? Also, would it be feasible to break some bigger chunks of code (e.g. find_diag function) into smaller functions with descriptive names?

893 + int diags = x_set_length + y_set_length + 3;
895 + // allocate one big buffer for the forward and backward vectors
896 + real_diag = new int[diags * 2];
897 + // offset the pointers, cause we're sharing the same mem block,
898 + // plus the indices can be negative
914 + while (diags != 0)
915 + {
916 + diags /= 4;
917 + max_cost *= 2;
918 + }
919 + max_cost = int.max (max_cost, 256);

There are a few magic numbers above - can you add comments describing them?
Also, 896 and the comment in 897-898 look a bit scary, are we certain it's never accessing array out of bounds?

881 + int* f_diag;
882 + int* b_diag;
883 + int max_cost;
884 + uint8[] real_changes;
885 + uint8* x_changes;
886 + uint8* y_changes;

Hmm... I was told by my vala teacher ;) that pointers in vala are evil?...

review: Needs Fixing

« Back to merge proposal