From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 10626 invoked by alias); 22 Oct 2003 23:16:43 -0000 Mailing-List: contact gdb-patches-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sources.redhat.com Received: (qmail 10619 invoked from network); 22 Oct 2003 23:16:42 -0000 Received: from unknown (HELO zenia.home) (12.223.225.216) by sources.redhat.com with SMTP; 22 Oct 2003 23:16:42 -0000 Received: by zenia.home (Postfix, from userid 5433) id AF2FC20766; Wed, 22 Oct 2003 18:16:31 -0500 (EST) To: Andrew Cagney Cc: gdb-patches@sources.redhat.com, Kris Warkentin , Daniel Jacobowitz Subject: Re: RFA: osabi: correct test for compatible handlers References: <3F96D128.5040904@redhat.com> <3F970598.9020908@redhat.com> From: Jim Blandy Date: Wed, 22 Oct 2003 23:16:00 -0000 In-Reply-To: <3F970598.9020908@redhat.com> Message-ID: User-Agent: Gnus/5.09 (Gnus v5.9.0) Emacs/21.3 MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-SW-Source: 2003-10/txt/msg00663.txt.bz2 Andrew Cagney writes: > > Andrew Cagney writes: > > > >> > + /* BFD's 'A->compatible (A, B)' functions return zero if A and B are > >> > + incompatible. But if they are compatible, it returns the 'more > >> > + featureful' of the two arches. That is, if A can run code > >> > + written for B, but B can't run code written for A, then it'll > >> > + return A. > >> > + + struct bfd_arch_info objects are atoms: that is, there's > >> > supposed > >> > + to be exactly one instance for a given machine. So you can tell > >> > + whether two are equivalent by comparing pointers. */ > >> > + return (a == b || a->compatible (a, b) == a); > > > >> Hey, nice. > >> Don't worry about a can_run_code_for function though, having the > >> logic > >> inline makes what's happening easier to understand (and will simplify > >> a follow-on wild-card patch I've got pending). > > It may be easier for you, but the original author did get the test > > backwards, and I had to go through an embarrassing number of wrong > > tries before I got it right. I'd really like to leave the function > > separate. > > I had to go through an equally enbarrassing number of tries before I > established exactly what the patch was doing. Changing: > > if (compatible == handler->arch_info) > > to: > > if (compatible == arch_info) > > (correct?) The really important thing here is your commentary as that > explains exactly what is going on. Having it as close as possible to > the problem (the call site) is, I think, going to make things easier > to understand. I agree it's harder to see what the *patch* does when I pull everything out into a separate function --- you have to expand the function in-place, and then compare before and after. But I think it's easier to see what the *resulting code* does with the function in place. We should put the readability of the resultant code above readability of the change. You say, "A can use a handler for B if A can run code for B", and then you can make a separate check to see whether can_run_code_for is correct.