Fix table.get("wrong_length_string") gives core dump #585
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Can one of the admins verify this patch? |
1 similar comment
|
Can one of the admins verify this patch? |
|
@mr-c ready for review! |
|
okay to test |
|
@mr-c I don't think the Travis bot quite triggered on this PR |
|
ok to test |
|
|
||
| if (strlen(s.c_str()) < counting->ksize()) { | ||
| PyErr_SetString(PyExc_ValueError, | ||
| "string length must >= the hashtable k-mer size"); |
There was a problem hiding this comment.
We don't use the word "hash" in front of users as it is an implementation detail. Yes, the Python API is going to get a lot of methods renamed :-)
Use "counting table" here instead.
…tent with other negative tests
Echelon9
force-pushed
the
fix/get-wrong_length_string
branch
from
2a67d76 to
b6a19da
Compare
|
Updated the PR with changes to those two user-visible output strings per review by @mr-c . |
|
Great work, @Echelon9. Thanks! |
|
Can one of the admins verify this patch? |
1 similar comment
|
Can one of the admins verify this patch? |
mr-c
added a commit
that referenced
this pull request
Sep 1, 2014
Fix table.get("wrong_length_string") gives core dump
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A number of
table.get("wrong_length_string")functions when called from Python would lead to an unhandled C++ exception, and a subsequent termination (Issue #174).Note: These reported paths to trigger the core dump included
table = khmer.new_hashbits(...),table = khmer.new_hashtable(...), andtable = khmer.new_counting_hash(...)Test Driven Development techniques were adopted in resolving this issue. Failing unit tests were written first based upon the user report, and then proposed patches committed until all the tests passed.
This methodology assisted in finding a second location in
khmer\_khmermodule.ccthat required the same fix. This was not immediately apparent upon first review of the C++ code.Testing the reproduction steps with gdb debugger and valgrind confirm the core dump is mitigated.
Check for code coverage.
make pep8,make diff_pylint_report,make cppcheck, andmake docoutput. Usemake autopep8,astyle -A10 --max-code-length=80, and manual fixing as needed.changes were made?