[m-dev.] Re: ODBC interface

[Fergus Henderson]

Simon Taylor, you wrote:
> Hi Fergus,
> 
> You wrote: 
> > Ah.  I see.  What do Mission Critical think of this?
> >
> > To distribute this, we would need their permission.
> 
> Michel Vanden Bossche, CEO of Mission Critical wrote:
> >> Do we have Mission Critical's permission to distribute this code
> >> under the GPL?

You asked the wrong question.  We'd like to distribute it under the
LGPL, not under the GPL.

> > >	# gcc aborts with optimization enabled.
> 
> Hmm, it doesn't any more.

> > >		if (rc != SQL_SUCCESS_WITH_INFO &&
> > >				! odbc_check(odbc_env_handle, 
> > >					odbc_connection, 
> > >					stat->stat_handle, rc)) {
> > >			odbc_do_cleanup_statement(stat);	
> > >			odbc_throw();
> > >		}
> > 
> > In cases like this, where the condition of an if statement wraps lines,  
> > it is IMHO clearer if the { is on a line of its own.
> > This indentation convention is described in our C coding guidelines.
> 
> You're probably right, but the only example of this in the C coding 
> guidelines is indented like this one.

Oh, you're right.  I *thought* it was that way in our C coding guidelines.
I think I'll fix them.

> > XXX you might need to change some of the pragma C code
> > from `will_not_call_mercury' to `may_call_mercury'.
> 
> Most of them are may_call_mercury. Which ones should be changed?

Ah, sorry, ignore that.

> > >	MR_ASSERT_IMPLY(connection_handle == SQL_NULL_HDBC, 
> > >			statement_handle == SQL_NULL_HSTMT);
> > >	MR_ASSERT_IMPLY(statement_handle != SQL_NULL_HSTMT, 
> > >			connection_handle != SQL_NULL_HDBC);
> > 
> > These two assertions both assert the same thing.
> 
> No, they are different:
> 	invalid connection handle => invalid statement handle
> 	valid statement handle => valid connection handle

But
 	invalid connection handle => invalid statement handle
implies
 	valid statement handle => valid connection handle

and indeed for all A, B

	(A => B) => ((not B) => (not A))

So although they are syntactically different, they are semantically
equivalent -- they both assert the same predicate.

	A	B	A => B		!A	!B	!B => !A
	t	t	t		f	f	t
	t	f	f		f	t	f
	f	t	t		t	f	t
	f	f	t		t	t	t

> #include ""odbc.h""

Hmm, doesn't odbc.c automatically include the automatically-generated
odbc.h file?  Is this statement necessary?
(Is there a conflict between the odbc.h file generated by Mercury
and some other odbc.h file?)

> 	/*
> 	** Mercury state to restore on rollback. 
> 	*/
> 	odbc_saved_hp = hp;
> 	odbc_saved_succip = succip;
> 	odbc_saved_maxfr = maxfr;
> 	odbc_saved_curfr = curfr;
> 	odbc_saved_sp = sp;

Wouldn't it be better if the saving and restoring of the Mercury registers
was done inside MR_setjmp() and MR_longjmp()?

> 	odbc_connection = (SQLHDBC) Connection;
> 	odbc_message_list = list_empty();
> 
> 	/* Set up a location to jump to on a database exception. */
> 	odbc_catch(transaction_error, do_transaction);
> 
> 	/*
> 	** Anything changed between the call to odbc_catch() and the call to
> 	** MODBC_odbc__do_transaction() must be declared volatile.
> 	*/
> 
> do_transaction:
> 	MODBC_odbc__do_transaction(TypeInfo_for_T, Closure, Results, DB0, &DB);

Hmm.  It took me a couple of minutes to figure out what was happening
here.

I think it might be clearer if odbc_catch() just took one label
as an argument, and in the ordinary case just fell through.

> 	/* 
> 	** Allocate an array of pointers to the info for each column
> 	*/
> 	statement->row = newmem((num_columns + 1) *
> 				sizeof(MODBC_Column *));

make_many() would be preferable to newmem().

> 		Int = (Integer) data;
> #ifdef DEBUG
> 		printf(""got integer %ld\\n"", Int);
> #endif /* DEBUG */

You need to cast Int to (long) in order to print it as "%ld".
There is no guarantee the `Integer' is the same size as `long'.

(I may have more comments later.)

-- 
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.