apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sander Striker" <stri...@samba-tng.org>
Subject RE: Memory code...
Date Thu, 10 May 2001 09:52:46 GMT
Hi again,

>> [.. apr_memory_system_free() ..]
>>     if (mem == NULL)
>>         return APR_EINVAL; /* Hmm, is this an error??? */
>>
>> Yes, this is an error, but not something you would want to fail
>> on (otherwise I would have used an assert). Maybe you could do
>> somekind of warning at debug time, something like:
>> 'Warning: NOOP caused by passing NULL pointer to apr_memory_free()'
>> I'm sure you can come up with something better.
>
> 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 them
> 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 more
> 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 library
(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.
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.

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 functions
>> 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 log
> errors is it?  We should simply report the problem and let the app decide
> 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??

> >
> > [.. apr_memory_system_is_ancestor() ..]
> >     /* strikerXXX: should this be: return b == a ? APR_TRUE : APR_FALSE;
> */
> >     /* 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 an
> 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.

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

Ok, I'll check the cvs list and try to get you a diff next time :-)

>>> Next on the list is a simple test program.
>>
>> Checking... apr/test/testmem.c
>>
>> Indeed, we need a more advanced test program, but this looks fine for
>> now. The simple addition one might do is adding a do_test_no_free()
>> and then call reset on the tracking memory system and again
>> do_test_no_free().
>> It shows reusability of tracking memory systems.
>
> Well, you can always submit a patch :)

*grin*

> david

Sander


Mime
View raw message