apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "David Reid" <dr...@jetnet.co.uk>
Subject Re: Memory code...
Date Thu, 10 May 2001 11:07:31 GMT
Hmm, replies inline.

> > Not our call is it?  We're a library, so if the user wants to abort on a
> > call of APR_EINVAL then that's their business.  We're simply telling
> > they passed in an invalid handle.  Chances are they'll ignore it.
> >
> > Roy's recent email about asserts in libraries makes interesting reading.
> > I'm in favour of removing a lot of the asserts once we've got things
> > stable and have "proved" the code more thoroughly.
> I've followed this one a bit too. I think you should make a distiction
> between users of a library(lets call them users) and developers of a
> (lets call them developers).
> For developers asserts are a great tool for finding bugs in the library.
> They really should not be removed.
> Furthermore, asserts are good to enforce certain user coding. Passing
> NULL pointers is generally a bad thing and checking for them is just
> overhead that can be avoided.

We only check for NULL pointers where they would cause segfaults and are
passed in due to an error.  I'd rather pass back an error to the caller so
that the code that has the error can be fixed.  In fact it may be that the
caller doesn't care and will happily carry on, not an option with an assert.

> I generally agree that some asserts could be removed or replaced, but
> for the rest I'm in the group of people that like code to bomb out
> and not silently fail or continue.

I agree about the asserts being useful for devlopers, which is why they're
still there, but once we have the code basically working I'm in favour of
reducing the quantity that we have in the code.  The use of asserts to
find/debug specific problems is good.

> Then again this is just my two cents...
> [...]
> >> When you find the time could you please change these comments to do
> >> as they say? :-) I didn't know what your error/warning logging
> >> look like (I reckon it is not fprintf(stderr, "some warning"); ), so
> >> I didn't actually implement it.
> >
> > Hmm, again we're a library, not an app...  It's not really our job to
> > errors is it?  We should simply report the problem and let the app
> > what to do about it.
> The trouble is that it is ok to use this code in this way. It will also
> succeed. It is just not needed, and therefor overhead. I would just like
> the users to know this so they can remove this call if they want. Maybe
> something like APR_VERBOSE as a compile flagg??

If it will succeed then what's the problem?  How do you get the information
to the users?  Look at the rest of the APR code.  We don't pass back to the
user anything but the return code.  In fact when this code is used by a
server (can we say Apache?) we'll never see the information anyway.  Why not
just add a note to the documentation?  Wouldn't that be easier?

> > >
> > > [.. apr_memory_system_is_ancestor() ..]
> > >     /* strikerXXX: should this be: return b == a ? APR_TRUE :
> > */
> > >     /* APR_SUCCESS = 0, so if they agree we should return that... */
> > >     return !(b == a);
> > >
> > > Ah, you guys don't have an apr_bool_t? Can I ask why not?
> > > Might I add this is a bit inconvenient, you cannot write this:
> > >   if (apr_memory_system_is_ancestor())
> > >     do_something();
> > >
> > > But then again, this might not be correct ASF coding style...
> >
> > No, but you can do
> >
> >   if (apr_memory_system_is_ancestor() == APR_SUCCESS)
> >     do_somehing();
> >
> > This is closer to what we have already, but we can always change it to
> > apr_int32_t...
> If you always enforce the '== APR_SUCCESS' it is ok. I'm just pointing
> out that for the users this might be confusing.

It's not that we enforce it, but that's the general style we've used
throughout APR.  Have you looked at the rest of the code?  We can always
change it to return an apr_int32_t as then we can use the logic you've
talked about.

> Maybe a stupid question, but why not introduce an apr_bool_t?


> [...]
> > All these will be addressed.  I'll try to get to as many of them as I
> Ok, I'll check the cvs list and try to get you a diff next time :-)

Yeah well, that would be good.


View raw message