apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From r..@covalent.net
Subject Re: cvs commit: apr/include apr_general.h
Date Tue, 02 Jan 2001 04:40:26 GMT

> > > the beginning of the structure, is just plain dangerous. The whole point of
> > > being incomplete is to hide their organization; that assumption shouldn't be
> > > made.
> > 
> > Every single APR incomplete type has a pool as the first field.  That is
> > part of the design, and it is documented in the APRDesign doc.  This was
> > purposefully done so that something like this macro could be implemented.
> 
> Heh. It's documented only cuz you put it in there today :-) Kind of like
> saying, "well, I can write ugly code because I just updated the design
> document to allow me to."
> 
> Just because you documented it, doesn't make it any safer.

Actually, it's documented today, because I wrote the docs today, I believe
this was mentioned in an e-mail a LONG time ago, when I first designed APR
and decided to implement it with incomplete types.  It's been this
way from the beginning however.  There has never been an APR incomplete
type that didn't begin with a pool.

> It is a non-typesafe hack.
> 
> We've spent quite a bit of time trying to increase type safety. Look at the
> original hooks, the new hooks from Ben, the macros for the directives, etc.

Yes, and those things could be implemented with type-safeness, without
affecting the amount of duplicated code, or maintainability.  Creating 12
different functions (one per APR type) to get access to the pool, which is
garaunteed to be in the same place in each structure.  This is just adding
code that we need to maintain.  Granted, they are easy functions, but why
do we need or want 12 of them?  Plus, it is just gets more complex as we
add more types.

> > > > b) we need this so that we don't leak memory like a sieve.  I committed
a
> > > > use of this to the buckets code a few minutes ago.
> > > 
> > > Then write apr_file_get_pool(). Not a cast and an assumption.
> > 
> > That also means that we have to write apr_socket_get_pool and
> > apr_lock_get_pool and apr_mmap_get_pool, etc.
> 
> Yup.

That is ugly and just plain wrong.

> > We control the type
> > definitions, and we control the macro.  This is very similar to what Dean
> > did with the ap_iol code.
> 
> 1) because it has been done before doesn't make it right
> 2) looking at ap_iol.h, I don't see what you are referring to. where did
>    this casting stuff occur?

There are three types, ap_iol, iol_socket, and iol_file.  iol_socket and
iol_file are two types that rely on the fact that the first field in their
structures was an ap_iol.  Basically, you pass an ap_iol into any of the
underlying functions, and it gets cast into the correct type.  From there,
we extract the ap_iol and the apr_socket_t or apr_file_t.  Look at
socket_setopt:

static apr_status_t socket_setopt(ap_iol *viol, ap_iol_option opt,
                               const void *value)
{
    iol_socket *iol = (iol_socket *)viol;

In these four lines, viol goes from being an ap_iol to being an
iol_socket, and this can only be done because of how each of them are
defined.

> Ryan: this is really not a good idea because of the type safety stuff.
> Please use accessor functions instead.

I completely disagree.  This is a perfectly valid C operation.  I have
taken mutliple functions that do nothing but return a field from a
structure, and just created a macro that can access that structure
directly.  The other option is one function per APR type that is basically
duplicated code.  We would have multiple functions that are essentially:

apr_pool_t *apr_get_pool_from_file(apr_file_t *f)
{
    return f->cntxt;
}

If I understand your problem with this, you are afraid that somebody will
use this macro on a variable that is either not an APR variable, or is an
APR type but is not one of the incomplete types.  Well, this can be fixed
multiple ways.  Just require that every APR structure has a pool at the
beginning of the structure, whether it is complete or not.  

I am as much against casting as anybody, but occasionally, it is the
correct solution.  IMNSHO, this is one of those times, and ruling it out
just because we lose a little bit of type-safeness seems a bit rash.  We
do not need to protect programmers from themselves in all cases.  We have
to assume that programmers are smart enough to read and understand the
docs.  If we carry the argument that we have to protect the programmer
from themselves far enough, then we end up checking every single APR
variable when it is passed into the APR function to ensure that it has
been created correctly.

Ryan

_______________________________________________________________________________
Ryan Bloom                        	rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------






Mime
View raw message