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 09:01:27 GMT
Hello.

> > As some of you will have noticed I've just added a first pass at
> > the memory
> > code from the Samba folks.  It's just a first pass but does build
cleanly,
> > when included in the build.  I've not yet added it to the apr_modules
line
> > in configure.in as until we know it's building on a few platforms I
didn't
> > want to add it there.  So, can people start checking the build?
>
> First question, why did put the code in a 'unix' subdirectory?
> As far as I know I only used standard c. It builds cleanly on win32
aswell.
> Does this mean creating another subdir 'win32' and copy the sources there
> too?
>
> As I said earlier, I haven't been tracking APR that long, so I don't know
> what conventions you guys use.

Try looking at the rest of the code in APR :)  Win32 can build in a Unix
directory, but eventualy there will be some custom Win32 code and so we'll
have a memory_common.c in the win32 directory that simply includes the code
in the unix directory.

>
> > Also, the code has been altered slightly to get it into APR, so can the
> > Samba folks check I haven't changed things too badly.
>
> Checking... apr/memory/unix/apr_memory_system.c (hmm, shortening the
> filename
> wouldn't hurt either :-)

I've posted to the list baout that.

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

>
> [.. apr_memory_system_free() ..]
>         /* issue a warning:
>          * WARNING: Called apr_memory_system_free() on a tracking memory
> system
>          */
>     }
>
> 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.

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

>
> [.. apr_memory_system_cleanup_register() ..]
>     /*
>      * If someone passes us a NULL cleanup_fn, assert, because the cleanup
>      * code can't handle it _and_ it makes no sense.
>      */
> +   assert(cleanup_fn != NULL);
>
>     cleanup = (struct apr_memory_system_cleanup *)
>
> apr_memory_system_malloc(memory_system->accounting_memory_system,
>                                      sizeof(struct
> apr_memory_system_cleanup));
>
>     /* See if we actually got the memory */
>     if (cleanup == NULL)
>         return APR_ENOMEM; /* strikerXXX: Should this become APR_FALSE? */
>
>     cleanup->data = data;
>     cleanup->cleanup_fn = cleanup_fn;
>     cleanup->next = memory_system->cleanups;
>     memory_system->cleanups = cleanup;
>
>     return APR_SUCCESS; /* strikerXXX: Should this become APR_TRUE? */
>
> Ok, we should add the assert line, because the comment says we do and we
> don't
> seem to actually do it :-)
> Second, you can remove my comments on the return lines, because this seems
> perfectly ok.

OK, will look at it.  I'd already removed a few of the comments.

>
> [.. apr_memory_system_cleanup_unregister() ..]
>             return APR_SUCCESS; /* strikerXXX: Should this become
APR_TRUE?
> */
>
> Same here, is ok, remove the comment.
>
>
> It just occurred to me that I could have send you a diff, but I wanted to
> comment on it anyway so I'm afraid I didn't :-)
>
> Checking... apr/memory/unix/apr_standard_memory_system.c
>
> #include <memory.h> /* strikerXXX: had to add this for windows to stop
>                      * complaining, please autoconf the include stuff
>                      */
>
> I think this can go. It was needed for memset() on win32.
>
> Checking... apr/memory/unix/apr_tracking_memory_system.c
>
> Looks ok.
>
> Maybe overall you might want to change comments, documentation, coding
> style.
>
> Checking... apr/include/apr_memory_system.h
>
> [..]
>  * @return TRUE if a is an ancestor of b, FALSE if a is not an ancestor of
b
>  * @deffunc apr_bool_t apr_memory_system_is_ancestor(apr_memory_system_t
*a,
>  *                                                   apr_memory_system_t
*b)
>  */
> APR_DECLARE(apr_status_t)
> apr_memory_system_is_ancestor(apr_memory_system_t *a, apr_memory_system_t
> *b);
>
> Let the documentation reflect the code. Ie. 'return APR_SUCCESS...' and
> '@deffunc apr_status_t ...'
>
> [..]
>  * @deffunc void apr_memory_system_destroy(apr_memory_system_t
> *memory_system)
>  */
> APR_DECLARE(apr_status_t)
> apr_memory_system_destroy(apr_memory_system_t *memory_system);
>
> Same here.
>
> Checking... apr/include/apr_tracking_memory_system.h
>
> [..]
> /**
>  * Create a tracking malloc/realloc/free memory system
>  */
>
> Ah, change this to:
>  * Create a tracking memory system
>
> My mistake :-), I just changed 'standard' to 'tracking' which is
> obviously incorrect.

All these will be addressed.  I'll try to get to as many of them as I can.

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

david



Mime
View raw message