[m-dev.] Re: ODBC interface

[Fergus Henderson]

Simon Taylor, you wrote:
> 	    case MODBC_FLOAT: {
> 
> 		MODBC_C_FLOAT data = *(MODBC_C_FLOAT *)(col->data);
> 
> 		Flt = (Float) data;
> #ifdef DEBUG
> 		printf(""got float %f\\n"", Flt);
> #endif /* DEBUG */
> 		break;
> 
> 			/* Check for overflow */
> 		if (Flt != data) {

This test is not an appropriate way of checking for overflow
for floating point numbers.  The test may also fail due to
loss of precision, so if `Float' is not the same type as `MODBC_C_FLOAT',
this test is likely to fail.

My guess is that most C compilers will do something reasonable
when converting float types, e.g. giving the result a value of
`Infinity' or `-Infinity' if it doesn't fit in the destination type,
or raising a floating point exception, so maybe it isn't necessary
to check for overflow for floats.

> 			restore_transient_registers();
> 			column->data = newmem(column->size);
> 			save_transient_registers();

These call to save/restore regs are currently not necessary,
because newmem() does not access the registers.

I think you may need more documentation regarding memory management.
Which data is allocated using (a) malloc/free (b) GC_malloc/GC_free
(c) newmem/oldmem (d) incr_hp, and why?

> 	incr_hp_atomic(this_bit, MODBC_CHUNK_SIZE / sizeof(Word));

Hmm, is MODBC_CHUNK_SIZE required to be a multiple of sizeof(Word)?
If not, shouldn't you round up rather than down?

> odbc__overflow_message(Error) :-
> 	Error = error(execution_error(overflow))
> 		- "[Mercury][odbc.m]Overflow detected.".

Hmm.  This error message could probably be improved.

> 	% The first argument should really be moded di.
> :- pred odbc__cleanup_statement_check_error(odbc__statement,
> 		odbc__state, odbc__state).
> :- mode odbc__cleanup_statement_check_error(in, di, uo) is det.

Re "the first argument should really be moded di":
if so, why isn't it?

> 		/*
> 		** 64-bit signed int converted to SQL_C_CHAR 
> 		*/
> 		case SQL_BIGINT:
> 			return 1 + cbColDef + 1;  /* (+ 1 == '\\0') */

OK, so one `1' is for the '\0', but what's the other `1' for?

> 		/*
> 		** Signed decimal ddd.dd converted to SQL_C_CHAR
> 		*/
> 		case SQL_DECIMAL:
> 			return 1 + cbColDef + 1 + ibScale + 1; 
> 						/* (+ 1 == '\\0') */

Ditto.

> 		/*
> 		** Signed numeric ddd.dd converted to SQL_C_CHAR
> 		*/
> 		case SQL_NUMERIC:
> 			return 1 + cbColDef + 1 + ibScale + 1;
> 					/* (+ 1 == '\\0') */

Ditto.

> 	; { odbc__no_data(Status) } ->
> 		% iODBC 2.12 doesn't implement this function.
> 		{ Messages = [
> 			error(feature_not_implemented) - 
> 			"[Mercury][odbc.m]SQLDataSources is not implemented by iODBC 2.12."

Hmm, the error message here seems to be precise but not necessarily
accurate.  You haven't checked whether they are using ODBC 2.12.
If they're not, then the message will be true but irrelevant and
misleading.

-- 
Fergus Henderson <fjh at cs.mu.oz.au>   |  "I have always known that the pursuit
WWW: <http://www.cs.mu.oz.au/~fjh>   |  of excellence is a lethal habit"
PGP: finger fjh at 128.250.37.3         |     -- the last words of T. S. Garp.