Odd tests in _khmermodule.cc

[pgarland]

Hello,

I was looking at learning about khmer and fixing some of the low-hanging fruit, and while taking a look at at #640, I started reading https://github.com/ged-lab/khmer/blob/master/khmer/_khmermodule.cc#L4499 and some of the tests for kmer size looked off to me.

I haven't written a lot of C++ and have little experience with the Python/C API, so maybe I'm missing something, but 2 of the functions I looked at (forward_hash and reverse_hash) contain tests for the size of k that don't seem to match up with their exception's error message, and one (forward_hash_no_rc) contains a test for the size of k that looks like it should always evaluate to false.

static PyObject * forward_hash(PyObject * self, PyObject * args)
{
    const char * kmer;
    WordLength ksize;

    if (!PyArg_ParseTuple(args, "sb", &kmer, &ksize)) {
        return NULL;
    }

    if ((char)ksize != ksize) {
        PyErr_SetString(PyExc_ValueError, "k-mer size must be <= 255");
        return NULL;
    }

    return PyLong_FromUnsignedLongLong(_hash(kmer, ksize));
}

Shouldn't the second if statement evaluate to true if ksize is greater than 127, not 255? That's what happened when I wrote a small test program on my computer using g++-4.9 on Linux on amd64. You can see this as well in python:

Python 2.7.8 (default, Oct 18 2014, 12:50:18)
[GCC 4.9.1] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> >>> >>> >>> import khmer
>>> khmer.forward_hash('A',128)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: k-mer size must be <= 255 

In the forward_hash_no_rc :

static PyObject * forward_hash_no_rc(PyObject * self, PyObject * args)
{
    const char * kmer;
    WordLength ksize;

    if (!PyArg_ParseTuple(args, "sb", &kmer, &ksize)) {
        return NULL;
    }

    if ((unsigned char)ksize != ksize) {
        PyErr_SetString(PyExc_ValueError, "k-mer size must be <= 255");
        return NULL;
    }
...
}

Shouldn't the second if statement here always evaluate to false?- since ksize is already an unsigned char, the cast shouldn't change its value and so both sides should always be equal.

In reverse_hash:

static PyObject * reverse_hash(PyObject * self, PyObject * args)
{
    HashIntoType val;
    WordLength ksize;

    if (!PyArg_ParseTuple(args, "Kb", &val, &ksize)) {
        return NULL;
    }

    if ((char)ksize != ksize) {
        PyErr_SetString(PyExc_ValueError, "k-mer size must be <= 255");
        return NULL;
    }

    return PyString_FromString(_revhash(val, ksize).c_str());
}

As with forward_hash, shouldn't the second if statement here evaluate to true if ksize is greater than 127, not 255? In python:

>>> khmer.reverse_hash(0, 128)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: k-mer size must be <= 255

Since the hash function throws a C++ exception anyways when ksize > sizeof(HashIntoType)*4, couldn't these functions use the same test and adjust the exception string accordingly?

[mr-c]

This was fixed in #663