Re: module cleanup [2/n]

[Eric Blake]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

According to Gary V. Vaughan on 9/11/2007 4:06 PM:
>>
>>     Wrap lt_dlhandle in struct m4_module.
> 
> Wrapping like this introduces an additional dereference every time a module
> is accessed.  For this, and aesthetic reasons, I'd rather see m4_module as
> a simple typedef alias.

libltdl can only attach a single pointer to the lt_dlhandle.  So it has to
be a pointer to a struct for us to store more information than that.  I'm
still not convinced whether handing m4 clients the raw lt_dlhandle (under
a type-safe opaque alias) is any more efficient than handing them the
pointer to the struct associated with the handle, and having that struct
point back to the handle (which is what I implemented).  One benefit of my
approach is that we can cache anything in the struct, reducing the need to
continually rely on libltdl accessors to relearn something that hasn't
changed since the last time we checked (for example, the implementation of
m4_get_module_name could be sped up by caching lt_dlgetinfo(handle)->name
in the m4_module, at which point a fast accessor macro is easier to write).

> 
>>     (m4_module): New declaration.  Simple for now, but intended for
>>     growth.
> 
> I would say that you have found a deficiency in the libltdl apis if you
> need
> to duplicate information that is already tracked by libltdl itself, or that
> you are unable to store with lt_dlcaller_set_data...

I found that storing the m4_module* using lt_dlcaller_set_data worked all
right, once I fixed the lt_dlcaller_get_data bug.

> 
>>     (m4_module_makeresident, m4_module_refcount): New functions.
> 
> These should be macros (at least with NDEBUG) that defer to the
> identical ltdl
> calls, to avoid the extra stack frame.

I'll keep that on my list of future cleanups.

> 
>>     (module_close): Delete, and inline into module_remove.
> 
> I rather liked the symmetry before this change :-(

This whole area is still a bit awkward - particularly since both functions
were having to strdup the module name in case of error.

You're right that there are still some symmetry issues to work out, as
well as correctly dividing the work between the three layers:

m4_module_import/m4_module_? - opens a module in order to import an entry
point from it (for example, libgnu's esyscmd needs to affect libm4's
sysval builtin) - this currently affects the m4 symbol table, but I'm not
sure that it should, and I don't know if a symmetric partner is needed

m4_module_load/m4_module_unload - opens modules and affects m4 symbol
table, designed for use by libload and other m4 modules

m4__module_open/m4__module_? - opens a module, but leaves m4 symbol table
alone, designed for use by internals, such as src/freeze.c

static module_?/static module_remove - handles the libltdl handle open and
close, designed for use only by module.c

I'll think about it some more, and try to clean it up better.

>         m4_module *module;
> 
> ...would make it easier to distinguish between handles (ld_dlhandle) and
> modules
> (m4_module *) on the boundaries between the ltdl and m4module code.

Thanks for doing that cleanup patch.

- --
Don't work too hard, make some time for fun as well!

Eric Blake             address@hidden
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.5 (Cygwin)
Comment: Public key at home.comcast.net/~ericblake/eblake.gpg
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFG6S+j84KuGfSFAYARAgM5AJ47lypL8ECv86uOmSf1c9MTE3FwEACg2QUd
Zmzwtq2dmW7dkD1DmOU8MHE=
=sd/H
-----END PGP SIGNATURE-----