Summary

Addressing TODO for faster algorithm to convert from file line numbers to filediff line numbers

Review Request #3980 — Created March 20, 2013 and discarded April 25, 2014, 1:58 p.m.

Information

Owner

soorajchirag

Repository

ReviewBot

Branch

Bugs

Depends On

Reviewers

Groups

reviewbot

People

smacleod

Description

Added a function _locate_line() that does a binary search for the chunk that contains the line to be searched and then follows up with another binary search within the chunk to get to the line itself. The function returns the chunk and the row which contains the line number.

Testing Done

Tested by running ReviewBot (cpplint, pep8 -- on 1 Cpp file and 1 python script) with the changes and the comments were updated against the correct line numbers in the file diff.

Issues

Description	From	Last Updated
This is returning None in some cases where it shouldn't be.	SM smacleod	May 27, 2013, 11:40 p.m.
Col: 1 W191 indentation contains tabs	reviewbot	May 29, 2013, 11:35 p.m.
Col: 1 E101 indentation contains mixed spaces and tabs	reviewbot	May 29, 2013, 11:35 p.m.
Col: 35 E702 multiple statements on one line (semicolon)	reviewbot	May 29, 2013, 11:35 p.m.

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    bot/reviewbot/processing/review.py
  Ignored Files:

Change Summary:

Fixed typo in Testing

Testing Done:

~		Tested by running ReviewBot (cpplint, pyflakes -- on 1 Cpp file and 1 python script) with the changes and the comments were updated against the correct line numbers in the file diff.
	~	Tested by running ReviewBot (cpplint, pep8 -- on 1 Cpp file and 1 python script) with the changes and the comments were updated against the correct line numbers in the file diff.

While testing this change I've found it fails to locate some line numbers it should be able to find. I've only seen it happen with unmodified lines, but haven't run nearly enough tests yet. I'm going to dig deeper into the logic here and try and figure out what's happening.

bot/reviewbot/processing/review.py (Diff revision 1)

The issue has been resolved. Show all issues

This is returning None in some cases where it shouldn't be.

soorajchirag April 8, 2013, 11:31 p.m.

Thanks for testing it out Steven. Do you have the test diff with which this failed? I could try to debug it from my end as well.

smacleod April 9, 2013, 11:12 a.m.

I've emailed you a diff and a script which can be used for testing. Let me know if you find anything.

soorajchirag May 27, 2013, 11:54 p.m.

Looks like "del(self.diff_data.chunks[mid_chunk])" is not having an effect on the list. As a result the index was getting modified but the list itself was not being updated. We ended up skipping past the chunks that had the line number we were looking for.

Interestingly (to me at least, someone well versed with python may be able to explain what's happening), if I take a reference to the list, and then call del on the mid_chunk the item gets deleted from the list.

I've tested the code with the script Steven shared with me:

sundeep@RBserver:~/Documents$ python rbotprofile.py 
sundeep@RBserver:~/Documents$ 

No errors reported.

Change Summary:

Addressing Steven's comment

Diff:

Revision 2 (+76 -21)

Show changes

bot/reviewbot/processing/review.py

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    bot/reviewbot/processing/review.py
  Ignored Files:

bot/reviewbot/processing/review.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 1
 W191 indentation contains tabs
```
bot/reviewbot/processing/review.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 1
 E101 indentation contains mixed spaces and tabs
```
bot/reviewbot/processing/review.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 35
 E702 multiple statements on one line (semicolon)
```

Change Summary:

Addressed Pep8 reported issues. Ran pep8 check locally before posting current diff

root@RBserver:~/Documents/git/reviewbot# pep8 bot/reviewbot/processing/review.py
root@RBserver:~/Documents/git/reviewbot#

Diff:

Revision 3 (+76 -20)

Show changes

bot/reviewbot/processing/review.py

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    bot/reviewbot/processing/review.py
  Ignored Files:

Review Board 7.0 alpha 0 (dev)

Addressing TODO for faster algorithm to convert from file line numbers to filediff line numbers

Information

Reviewers

Change Summary:

Testing Done:

Change Summary:

Diff:

Change Summary:

Diff:

Status: Discarded