Re: module cleanup [1/n]

[Eric Blake]

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

According to Gary V. Vaughan on 9/11/2007 1:42 PM:
> Hi Eric,

Hi Gary, and thanks for the review.

> Errors should be diagnosed as early as possible, and a script that loads
> a module
> must do so for additional builtins.  Better to exit with a failed load
> early, than
> to quietly copy text badly due to a missing builtin that causes
> something not to
> expand.
> 
> Perhaps we could add an options parameter to load() like this?
> 
>    load(`nosuchmodule', `nocomplain')

I'm thinking more like:

load(`nosuch') == load(`nosuch', `fatal') => die immediately

load(`nosuch', `error') => issue a warning to stderr, affect exit status,
but proceed (unless -EE specified on command line)

load(`nosuch', `warn') => issue a warning to stderr, leave exit status
alone (unless -E specified on command line)

load(`nosuch', `silent') == load(`nosuch', `quiet') => like sinclude -
proceed without warning and without affecting exit status

> Agreed.  The module interface has evolved through several iterations in
> previous
> alpha releases, and it is clearly marked as experimental in README
> and/or NEWS
> IIRC, so further improvements at the expense of backward compatibility
> are quite
> acceptable prior to release IMHO.

OK, then I won't even worry about distinguishing earlier modules.

> 
> It certainly can't do any harm to be more careful about this.  HEAD
> libltdl does
> assume that only one module loading extension point will be responsible
> for loading
> of any module.  That is, if a module is written to be loaded by the m4
> module loader,
> then it shouldn't be loaded by a different module system (say the
> libsnprintfv
> extension point -- if we reimplemented m4's format builtin using
> libsnprintfv).

Really?  I thought the whole point of adding the lt_dlinterface_id was to
allow multiple loaders.  Anyways, more on this in response to 3/n.

> 
> I'm loathe to keep adding builtins where we can extend existing builtins
> to get
> the same functionality in a clean intuitive way.  m4modules is not yet
> bound to
> provide backwards compatibility, so we could simply have it add newly
> loaded modules
> to the list each time they are successfully loaded.  Membership tests
> would be
> unaffected, and a macro to count occurences could easily work out the
> refcount of
> a named module.

True.  But the current m4modules implementation is quite dependent on the
libltdl traversal algorithm (currently stack-based, but lists multiply
loaded modules only once and in the order they were first loaded).
Changing m4modules to list every load of every module seems nicer than
having a new 'refcount' builtin, so I can revert that part of the patch,
but it means m4modules will now have to maintain an explicit list of
module loads, rather than relying on libltdl's list.

>> - If we add symbol caching, then it becomes trivial to store information
>> associated with a builtin (back to my original thought of adding the
>> ability to
>> trace a builtin regardless of the name it is invoked by); without a
>> cache,
>> there is no convenient place to do per-builtin tracking since we can't
>> necessarily write into the memory owned by a module
> 
> Libltdl has APIs to attach data structures to a loaded module.  That may
> or may
> not prove to be the easier place to hang things...

That's already what I used (after fixing the libtool bugs in that area!).
 Basically, I hang the m4_module* off of the libltdl handle, then we are
free to cram whatever we want into the m4_module structure, all reachable
via the libltdl handle.

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

iD8DBQFG6Swy84KuGfSFAYARAqp5AKCyXf4+4mexDmF0kn+1BModQ4YkJwCfZraw
axckne/Hgjd2zasNxGOuXwA=
=dtW+
-----END PGP SIGNATURE-----