-1 until you put that GDBM support back in. It isn't right to just drop it.
On Mon, Aug 20, 2001 at 04:50:25PM -0700, Ian Holsman wrote: > On Mon, 2001-08-20 at 13:07, Sterling Hughes wrote: >... > > Not sure I understand what you're saying? There are ways to allow > > the registering of module functionality, without modifying the main > > package. Just provide a function like: > > > > apr_dbm_register() > > > > Which registers a dbm internally, and then have a constant defined > > in a seperate file, and all is clear... > that would work, > but that wouldn't that mean there would be need to some init code > somewhere to call the registration function? Every time we load a module, we call registration functions. That is no big deal. Your solution of apr_dbm_*_open() still requires a recompilation/relink. The registration mechanism is completely open-ended. > > > 2. I may have a dbm which needs extra parameters to open it. (say for > > > exampe cache size, or maybe it requires > > > a fixed key length defined) I couldn't do this simply > > > > > > > dbm_set_x() and check the flags, once they're all filled out (a > > va_args implementation could also be done...) Not sure what you mean by dbm_set_x() here. You have to open the thing first to get an apr_dbm_t. But then you're too late. The va_arg solution is a semi-reasonable solution, but that kills any chance for type checking. > > Furthermore, the idea of an abstraction layer is too bring all these > > down to a lowest common denominator, at the cost of functionality > > sometimes; usually functions that take extra arguments, can be > > filled in with acceptable defaults. You'll run into the same > > problems in that some db's don't support certain features, and > > others do, the idea is to concentrate on the functionality that is > > necessary and shared between the different db's (wrap this in a > > large imho :). Exactly. apr_dbm exposes DBM-style functionality. Simple key/value pairs. That is it. The open functionality does not need flexibility. It has not been demonstrated yet that we need more capability there. > true, thats why they all return the same thing (apr_dbm_t) > the open_DBM function was supposed to be called when the devloper wanted > to do something specific with this type of dbm. so in the berkley DB > case there could be a apr_dbm_db_getRaw() function which returns the DB* > so that developers are not constrained the set of functions we chose to > abstract. No... we already have patterns for getting at underlying data types. See apr_portable.h. Follow that pattern *IF* we are to expose those. IMO, we should not do so. > (BTW I was planning on implmenting a shared-memory hash table, that I > posted ages ago, via this DBM interface, and it required a different set > of opening parameters (key/elem size, #elements) these parameters could > be passed via a db_set function call but it would look clunky No... keys and values are arbitrary length, as specified by an apr_datum_t. If your hash cannot handle that, then it cannot be used in the APR DBM interface. >... > so.. > shall we put it to a vote? > > apr_dbm_open_XXX > or > apr_dbm_open(XXX) Registration and the second form. But your patch still has lots of work anyways. The GDBM functionality can't just disappear. Sorry, but the DAV functionality uses apr_dbm and you'd completely break access to the property files. Second, the vtable should not be part of the runtime apr_dbm_t structure. You should have a pointer to a static const table, like we do with buckets. The formatting may have gone thru "indent" but it still sucks. Just a glance over it shows some random gunk. For example, what the heck is up with the formatting of apr_datum_t in apr_dbm.h? And all the spacing of the function params in apr_dbm.h has been changed. There are extra blank lines at the end of some functions (after returns), and some missing blank lines between functions. We should also see a patch against apr_dbm.h, rather than the whole file. I can't see if you've changed the public interface or not. dbm_private.h is missing a license and disclaimer. It also has a non-standard #ifndef/#define/#endif pattern around it. We never start the symbol with "_", and on all the new .h files you specified: toss the "1" from the #define ... we don't do that either. Why is the "used names" functionality part of the per-db public interface? That should be part of the hook stuff just like the rest. In any case... the per-db headers should go, in favor of the registration mechanism. You would register each DB style with a name. You can then open a DB style by name, or using the reserved "any". Note that we would define some standard names: #define APR_DBM_USE_SDBM "sdbm" #define APR_DBM_USE_GDBM "gdbm" #define APR_DBM_USE_DB "db" #define APR_DBM_USE_ANY "any" Also, the whole point of this exercise was to link in all of the variations. That means the .c files should not have the #if APU_USE_*/#endif stuff around them. The APU_USE_* symbols are only used to determine which DB to use for the "any" case. So... -1 for now. The patch still needs work before it can be applied. Cheers, -g -- Greg Stein, http://www.lyra.org/
