From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21948 invoked by alias); 25 Nov 2008 17:47:21 -0000 Received: (qmail 21886 invoked by uid 22791); 25 Nov 2008 17:47:20 -0000 X-Spam-Check-By: sourceware.org Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 25 Nov 2008 17:46:27 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 310A52A966F; Tue, 25 Nov 2008 12:46:17 -0500 (EST) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id wSdH9drVMSB8; Tue, 25 Nov 2008 12:46:17 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id E71212A9654; Tue, 25 Nov 2008 12:46:15 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id C1AAAE7ACD; Tue, 25 Nov 2008 09:46:13 -0800 (PST) Date: Wed, 26 Nov 2008 06:34:00 -0000 From: Joel Brobecker To: Tom Tromey Cc: Jie Zhang , gdb-patches@sourceware.org Subject: Re: PING: [PATCH] Fix a bug of addrmap Message-ID: <20081125174613.GF3946@adacore.com> References: <48FD5DE1.9090105@analog.com> <490E9540.7000207@analog.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.4.2.2i Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2008-11/txt/msg00697.txt.bz2 > 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 > > * 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