Skip to content

Extra fix to unified_difft::lcss to not error on iterator check #1662

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 8, 2017
Merged

Extra fix to unified_difft::lcss to not error on iterator check #1662

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions src/goto-diff/unified_diff.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,8 @@ unified_difft::differencest unified_difft::lcss(
{
differences.push_back(differencet::NEW);
--j;
--new_rit;
if(new_goto_program.instructions.begin()!=new_rit)
--new_rit;
}
else if(j == 0)
{
Expand All @@ -273,15 +274,18 @@ unified_difft::differencest unified_difft::lcss(
{
differences.push_back(differencet::SAME);
--i;
--old_rit;
if(old_goto_program.instructions.begin()!=old_rit)
--old_rit;
--j;
--new_rit;
if(new_goto_program.instructions.begin()!=new_rit)
--new_rit;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am starting to wonder about the other cases of new_rit and old_rit being decremented, and do think that all of those will need the same adjustment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would agree. I have a version with all other cases in this loop guarded. However, I have only PR the cases that actually error in test cases and as I have new test case I discovered new error. Would you like me to amend the commit with the complete version?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Would it be possible to add those test cases somewhere?
  2. Yes, I'd say add all those checks, this isn't such a huge performance risk.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Possibly but it will take more time as they are on different repository.
  2. Commit amended to have all cases in the loop checked.

}
else if(lcss_matrix[i][j - 1] < lcss_matrix[i][j])
{
differences.push_back(differencet::DELETED);
--i;
--old_rit;
if(old_goto_program.instructions.begin()!=old_rit)
--old_rit;
}
else
{
Expand Down