New bindings leak C FFI types into the public API

[nurpax]

I wonder if this was intentional in @joeyadams new code:

newtype ParamIndex = ParamIndex CInt
    deriving (Eq, Ord, Enum, Num, Real, Integral)

This specifies ParamIndex to be a CInt instead of just an Int. This has surprising consequences, like:

-- in sqlite-simple code that uses direct-sqlite
  ParamIndex stmtParamCount <- bindParameterCount stmt
  when (length qp /= stmtParamCount) (throwColumnMismatch qp stmtParamCount)

Leads to a type error:

Database/SQLite/Simple.hs:115:22:
    Couldn't match expected type `Int'
                with actual type `Foreign.C.Types.CInt'
    In the second argument of `(/=)', namely `stmtParamCount'
    In the first argument of `when', namely
      `(length qp /= stmtParamCount)'
    In a stmt of a 'do' block:
      when
        (length qp /= stmtParamCount)
        (throwColumnMismatch qp stmtParamCount)

stmtParamCount here will be a CInt which at least to me was a surprise. I would expect counts like these to just use normal Ints, rather than types that are used for C bindings.

I think this will surprise users.

IMO the direct-sqlite public API should use just Ints, not CInts.

[nurpax]

It'd be good to decide what to do about this before the next release, as this relates to the public API.

[nurpax]

Hmm.. Now that I used it a bit more, it doesn't seem too bad, actually. Just need a few fromIntegrals here and there.

That being said, I feel like this is a bit leaky.

[IreneKnapp]

Yeah, I don't like it as CInt, but oh well, since I released before I saw this. I do think the CInt to Int change should happen and before next time,whenever that is.

Sent from my iPad

On Sep 28, 2012, at 11:38 AM, Janne Hellsten notifications@github.com wrote:

I wonder if this was intentional in @joeyadams new code:

newtype ParamIndex = ParamIndex CInt deriving (Eq, Ord, Enum, Num, Real, Integral) This specifies ParamIndex to be a CInt instead of just an Int. This has surprising consequences, like:

-- in sqlite-simple code that uses direct-sqlite ParamIndex stmtParamCount <- bindParameterCount stmt when (length qp /= stmtParamCount) (throwColumnMismatch qp stmtParamCount) Leads to a type error:

Database/SQLite/Simple.hs:115:22: Couldn't match expected type Int' with actual typeForeign.C.Types.CInt' In the second argument of (/=)', namelystmtParamCount' In the first argument of when', namely (length qp /= stmtParamCount)' In a stmt of a 'do' block: when (length qp /= stmtParamCount) (throwColumnMismatch qp stmtParamCount) stmtParamCount here will be a CInt which at least to me was a surprise. I would expect counts like these to just use normal Ints, rather than types that are used for C bindings.

I think this will surprise users.

IMO the direct-sqlite public API should use just Ints, not CInts.

— Reply to this email directly or view it on GitHub.

[IreneKnapp]

Hmm, I had forgotten about this old issue. I think I agree that Int would be better than CInt. Now we just need to remember to do it before 3.0 sometime. :)