From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 16513 invoked by alias); 25 Nov 2015 22:47:41 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 16499 invoked by uid 89); 25 Nov 2015 22:47:40 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.1 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Wed, 25 Nov 2015 22:47:39 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (Postfix) with ESMTPS id 50FD23D495 for ; Wed, 25 Nov 2015 22:47:38 +0000 (UTC) Received: from [10.3.233.69] (vpn-233-69.phx2.redhat.com [10.3.233.69]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id tAPMlbPi004478; Wed, 25 Nov 2015 17:47:37 -0500 Message-ID: <1448491656.19594.253.camel@surprise> Subject: Re: [PATCH 01/15] Selftest framework (unittests v4) From: David Malcolm To: Bernd Schmidt Cc: gcc-patches@gcc.gnu.org, Jeff Law Date: Wed, 25 Nov 2015 22:53:00 -0000 In-Reply-To: <564E1881.3030306@redhat.com> References: <564A1DAB.1030700@redhat.com> <1447952699-40820-1-git-send-email-dmalcolm@redhat.com> <1447952699-40820-2-git-send-email-dmalcolm@redhat.com> <564E084A.20006@redhat.com> <1447956495.19594.140.camel@surprise> <564E1881.3030306@redhat.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2015-11/txt/msg03159.txt.bz2 On Thu, 2015-11-19 at 19:44 +0100, Bernd Schmidt wrote: > On 11/19/2015 07:08 PM, David Malcolm wrote: > > gcc_assert terminates the process and no further testing is done, > > whereas the approach the kit tries to run as much of the testsuite as > > possible, and then fail if any errors occurred. > > Yeah, but let's say someone is working on bitmaps and one of the bitmap > tests fail, it's somewhat unlikely that cfg will also fail (or if it > does, it'll be a consequence of the earlier failure). You debug the > issue, fix it, and run cc1 -fself-test again to see if that sorted it out. > > As I said, it's a matter of taste and style and I won't claim that my > way is necessarily the right one, but I do want to see if others feel > the same. > > > The patch kit does use a lot of "magic" via macros and C++. > > > > Taking registration/discovery/running in completely the other direction, > > another approach could be a completely manual approach, with something > > like this in toplev.c: > > > > bitmap_selftest (); > > et_forest_selftest (); > > /* etc */ > > vec_selftest (); > > > > This has the advantage of being explicit, and the disadvantage of > > requiring a bit more typing. > > (possibly passing in a "selftest *" param if we're doing the > > try-to-run-everything approach, so we can count failures etc without > > introducing globals) > > > > Was that what you had in mind, or something else? > > It's one option, but it doesn't seem like the best one either. I was > thinking of something not dissimilar to your approach, but with fewer > bells and whistles. My class registrator would look something like this: > > static list test_callbacks; > > class registrator > { > public: > registrator (void (*)() cb) > { > test_callbacks.push_front (cb); > } > } > > (or use a vec if you can do that at global constructor time) > > and then you just walk the list and run the callbacks when you want to > run tests. The one you have implements both the registration and a > special case linked list, which just doesn't look right to me, FWIW, the reason I special-cased the linked list was to avoid any dynamic memory allocation: the ctors run before main, so I wanted to keep them as simple as possible. Putting the linked list directly into those objects means that running the ctors is a simple case of wiring up some pointers: the memory is already statically allocated. (also, one thing I want to test is vec<> itself [1]). Anything more complicated feels to me like asking for trouble - and as noted, I already have some bugs to track down with the linker apparently optimizing away some tests. (Maybe auto-registration is more trouble than it's worth?) > and I think I'd also have avoided the runner class. The runner class was mostly about accumulating pass/fail information, which goes back to the abort-on-first-failure vs try-to-run-everything question. If we do want the latter, it could be global state if that would be simpler (though I prefer to avoid global state). Dave [1] although to be fair, I used vec<> in one place within the patch, when sorting the tests.