Skip to content
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.

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

DataHandle: findString(..) add support handles with unknown length #339

Merged
merged 2 commits into from
Oct 16, 2018

Conversation

gab1one
Copy link
Contributor

@gab1one gab1one commented Oct 15, 2018

  • The findString(..) method used to require the exact length of the
    handle, this length can be unknown, e.g. for compressed handles.
    This is no longer the case

Copy link
Member

@ctrueden ctrueden left a comment

Choose a reason for hiding this comment

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

Just a couple of quick questions:

  • How much effort would be required to add a test? If easy, please do it.
  • Did you verify same performance? From the patch it does not appear there would be an issue, but good to be certain.

@gab1one gab1one changed the title DataHandle: fix findString(..) and support handles with unknown length DataHandle: findString(..) add support handles with unknown length Oct 15, 2018
@gab1one
Copy link
Contributor Author

gab1one commented Oct 15, 2018

@ctrueden regarding the testing, I encountered this bug when working on #274, there I was able to test it as well.

@gab1one gab1one force-pushed the improve-find-string branch from f0780ac to 6cf9dd9 Compare October 15, 2018 15:06
The findString(..) method used to require the exact length of the
handle, this length can be unknown, e.g. for compressed handles.
This is	no longer the case.
@gab1one gab1one force-pushed the improve-find-string branch from 6cf9dd9 to 4a365aa Compare October 15, 2018 15:11
@gab1one
Copy link
Contributor Author

gab1one commented Oct 15, 2018

I just cleaned the code up a bit more, now it should be more clear and length() is not used anywhere.

@ctrueden
Copy link
Member

Thanks @gab1one, but my previous two questions still stand:

  1. Would it be possible to add a regression test for findString, which failed previously, but passes now?
  2. Did you do any performance checking?

@gab1one
Copy link
Contributor Author

gab1one commented Oct 15, 2018

  1. I can try building such a test, it would require a handle that reports it's length wrongly.
  2. I did not, but I did not change how the actual processing logic work, basically I just moved the reading into the while loop head and used the number of read bytes as a EOF signal instead of the length reported by the handle. I can try to do some performance testing together with 1).

@ctrueden
Copy link
Member

it would require a handle that reports it's length wrongly.

Why? I thought the issue was for handles that do not know their own length. As in: they return -1? Did I misunderstand?

I did not change how the actual processing logic work

Fair enough; let's not worry about it then.

@gab1one
Copy link
Contributor Author

gab1one commented Oct 16, 2018

Yes ones that return -1. I added a test that reproduces the bug and discovered another one :), BytesHandle did not return -1 on EOF, see #341

@ctrueden ctrueden merged commit a922e15 into master Oct 16, 2018
@ctrueden ctrueden deleted the improve-find-string branch October 16, 2018 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants