Test failure about direct-sqlite HOT 8 CLOSED

Thank you for reporting this, and sorry about the slow reply. I will look into this over the weekend; poke me if you haven't heard back by Monday.

from direct-sqlite.

Note to say still alive, no poke needed. :)

from direct-sqlite.

This is indeed a 64-bit issue, but not IMO a very serious one.

The offending line in the test is:

              Nothing   <- columnName stmt (ColumnIndex minBound)
              Nothing   <- columnName stmt (ColumnIndex maxBound)

minBound in ColumnIndex evalutes to -9223372036854775808. However, sqlite3 column name function takes in a 32-bit int: const char *sqlite3_column_name(sqlite3_stmt*, int N); (http://www.sqlite.org/c3ref/column_name.html). The minBound value truncates to zero as a 32-bit int:

Prelude Data.Int> (-9223372036854775808) :: Int32
0

The maxBound using test cases have a similar truncation problem, it just doesn't trigger a test failure here, probably due to (0x7FFFFFFFFFFFFFFF & 0x7FFFFFFF == still a large number).

I'm not sure what'd be the best way to fix this. In C/C++ I think I'd just add an assert into the bindings not to use such a large column index. Or I might call "error" in

instance FFIType ColumnIndex CColumnIndex where
    toFFI (ColumnIndex n) = CColumnIndex (fromIntegral n)
    fromFFI (CColumnIndex n) = ColumnIndex (fromIntegral n)

if the input ColumnIndex is out of the 32-bit integer range. Or to satisfy the test, we could just clamp to range [-2^31, 2^31 - 1] in toFFI. I wouldn't want to expose this error condition in the upper levels of the API as then a corner case like this would need to be handled in too many places.

Or we can do nothing and change the test to not use minBound, maxBound but hardcode 32-bit integer values instead.

Note: Many other functions are affected by this, including very frequently called ones like column. I'd vote for a cheap (as in performance) solution to this, perhaps just changing the test case.

@joeyadams - thoughts? I think the test comes from one of your commits.

from direct-sqlite.

Thank you for the diagnosis, Janne! Yeah, it's indeed from Joey's commit, that's why I've been procrastinating on it. :)

If it's indeed a 32-bit value always at the C layer, then I see two options.

ColumnIndex is a newtype wrapping an Int, but it's not opaque, so if we were to change its content type to Word32, that would be a breaking interface change. But it would give us all the static safety.

I note that the test code calls ColumnIndex maxBound, since there is no Bounded instance for ColumnIndex, which is the reason it runs into problems. Could we just provide a custom Bounded instance for ColumnIndex which does fromIntegral ({min|max}Bound :: Word32)?

I'm leaning towards the second option, because breaking the interface over a hypothetical failure seems a little silly. But what does everyone think?

from direct-sqlite.

I like the latter approach! So the only thing it does is that minBound and maxBound will evaluate to values that fit into 32-bit ints for the FFI API? I'll send a pull request.

from direct-sqlite.

Merged. :) Can't do a release right at this moment, as I'm on a deadline, but will in the morning.

from direct-sqlite.

@IreneKnapp Can you release this fixed version? Would be nice to have tests working also on the Hackage version. Stackage is currently not running direct-sqlite tests because of this.

from direct-sqlite.

Of course, sorry. It's done!

from direct-sqlite.