Re: PING: [PATCH] Fix a bug of addrmap

[Joel Brobecker <brobecker@adacore.com>]

> Jie> Could someone give a review on this patch:
> Jie> http://sourceware.org/ml/gdb-patches/2008-10/msg00503.html
> 
> I was curious about this patch so I took a look.

I took a quick look too, but not being familiar with this part
of the code, I had to postpone the review until I find enough
time to familiarize myself with the code.

Did you familiarize yourself with the code that you'd say that
the patch sounds good to you? If this is the case, I'm quite
happy to trust your judgement.

> Since addrmap is a relatively self-contained data structure, I figured
> it was a good candidate for a unit test.  So, I wrote a test case
> based on your original report.

Yay! :)

> 2008-11-25  Tom Tromey  <tromey@redhat.com>
> 
> 	* Makefile.in (test-addrmap.o): New target.
> 	(test-addrmap): Likewise.
> 	(clean mostlyclean): Remove test-addrmap.
> 	* addrmap.c (xfree): New function.
> 	(internal_error): Likewise.
> 	(test_inclusion): Likewise.
> 	(main): Likewise.

The one comment that I have about this is that it's not automatically
run when we run the testsuite. It would be really nice if they were.

It's not the first time that we have unit-testing like this.
For instance, check we have unit tests for observers, or xfullpath.
Traditionally, they have been implemented inside testsuite/gdb.gdb.
The current solution is not very elegant, in my opinion, as what we
did was debugging GDB and call some functions inside GDB.

How about, we add a testcase that "debugs" test-addrmap by simply
running it to completion and checking its output (including the
fact that it exited normally (meaning error code returned is zero)?

> +void
> +xfree (void *ptr)
> +{
> +  free (ptr);
> +}
> +
> +/* Likewise for internal_error.  */
> +NORETURN void
> +internal_error (const char *file, int line, const char *string, ...)
> +{
> +  /* The user will have to debug this anyway.  */
> +  exit (1);
> +}

I really like the idea of compiling this file as a standalone program,
and I hope that we'll be able to extend the use of this approach in
the future.  If we do, it might be worth putting these in a unit-testing
archive... (just thinking aloud, let's not act on it until it becomes
useful).

-- 
Joel