On Wed, Apr 25, 2007 at 01:18:08PM +0200, H.Merijn Brand wrote:
> On Wed, 25 Apr 2007 09:54:34 +0100, Tim Bunce <[EMAIL PROTECTED]> wrote:
>
> > On Mon, Apr 23, 2007 at 08:56:45PM -0400, Patrick Galbraith wrote:
> > > Hi all,
> > >
> > > Soon, I'll be releasing DBD::mysql 4.005 and was wondering if there any
> > > last minute thoughts, requests?
> >
> > A few general 'DBI best practice' items (that other drivers authors
> > should check their code for)...
>
> Good for immediate checking :)
> Some short questions to check if I understood that all right ...
>
> > ---
> > Don't use DBIS or dbis when you can use DBIc_STATE (imp_xxh).
> > DBIS is much slower, especially on perls built with threads enabled.
> > It's a macro that expands to a function call, whereas
> > DBIc_STATE (imp_xxh) is just accessing a field in the handle structure.
>
> This does not apply to?:
>
> av = DBIS->get_fbav (imp_sth);
> num_fields = AvFILL (av) + 1;
Yes. Ideally the code should have no DBIS (or dbis) in it at all. The only
exception would be any code that doesn't have a handle/imp_xxh handy.
> > In the specific case of dbis->debug or DBIS->debug, use
> > DBIc_TRACE_LEVEL (imp_xxh) instead.
>
> What if the debug function that uses dbis does not have a handle
> in it's visible arguments?
Add one :)
> > That correctly masks out the higher bits that DBI now reserves
> > for trace flags. See TRACING in the DBI docs.
>
> So, this:
>
> for (i = 0; i < n; i++) {
> imp_sth_t *imp_sth = children[i];
> if (2 > dbis->debug && 2 > opt_v) {
>
> would change to this?:
>
> for (i = 0; i < n; i++) {
> imp_sth_t *imp_sth = children[i];
> if (2 > DBIC_TRACE_LEVEL (imp_sth) && 2 > opt_v) {
Yeap.
> > ---
> > DBIc_ERR and DBIc_ERRSTR, and DBIc_STATE should not be set directly.
> > So change do_error () from the existing 28 lines to something like this:
> >
> > void do_error (SV *h, int rc, const char *what, const char *sqlstate)
> > {
> > D_imp_xxh (h);
> > DBIh_SET_ERR_CHAR (h, imp_xxh, Nullch, rc, what, sqlstate, Nullch);
> > }
> >
> > That will call set_err () to correctly merge info, warning and error states
> > and trigger the $h->{HandleSetErr} hook. See set_err () in the DBI docs.
>
> So, this:
>
> static void error (SV *h, int error_num, char *text, char *state)
> {
> D_imp_xxh (h);
> sv_setiv (DBIc_ERR (imp_xxh), (IV)error_num);
> sv_setpv (DBIc_ERRSTR (imp_xxh), text);
> unless (state)
> sv_setpv (DBIc_STATE (imp_xxh), state);
> } /* error */
>
> would change to this?:
>
> static void error (SV *h, int error_num, char *text, char *state)
> {
> D_imp_xxh (h);
> DBIh_SET_ERR_CHAR (h, imp_xxh, Nullch, error_num, text, state, Nullch);
> } /* error */
>
> and still do the sam
The same, and much more. See set_err() in the DBI docs.
Which reminds me, in the perl code drivers should set error conditions
with code like
return $h->set_err($err, $errstr, $state) if $something_went_wrong;
> > The do_warn code is confused. It sets DBIc_ERR to a true value which
> > means it's really acting like do_error (it'll trigger RaiseError etc).
> > It should probably look like this:
> >
> > void do_warn (SV *h, int rc, char *what)
> > {
> > D_imp_xxh (h);
> > DBIh_SET_ERR_CHAR (h, imp_xxh, "0", 0, what); /* "0" = warning */
> > }
>
> I have no do_warn () code in my DBD. Should I have?
No need for do_warn, specifically. But using DBIh_SET_ERR_CHAR (or set_err)
to report Warning (err="0") and Information (err="") states may be useful.
Tim.
