From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 26801 invoked by alias); 28 Sep 2012 04:00:58 -0000 Received: (qmail 26793 invoked by uid 22791); 28 Sep 2012 04:00:57 -0000 X-SWARE-Spam-Status: No, hits=-3.3 required=5.0 tests=AWL,BAYES_00,KHOP_THREADED,RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from sibelius.xs4all.nl (HELO glazunov.sibelius.xs4all.nl) (83.163.83.176) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 28 Sep 2012 04:00:52 +0000 Received: from glazunov.sibelius.xs4all.nl (kettenis@localhost [127.0.0.1]) by glazunov.sibelius.xs4all.nl (8.14.5/8.14.3) with ESMTP id q8S40kCE019435; Fri, 28 Sep 2012 06:00:46 +0200 (CEST) Received: (from kettenis@localhost) by glazunov.sibelius.xs4all.nl (8.14.5/8.14.3/Submit) id q8S40iKO021106; Fri, 28 Sep 2012 06:00:44 +0200 (CEST) Date: Fri, 28 Sep 2012 04:00:00 -0000 Message-Id: <201209280400.q8S40iKO021106@glazunov.sibelius.xs4all.nl> From: Mark Kettenis To: brobecker@adacore.com CC: siddhesh@redhat.com, gdb-patches@sourceware.org In-reply-to: <20120926105205.GB4335@adacore.com> (message from Joel Brobecker on Wed, 26 Sep 2012 12:52:06 +0200) Subject: Re: [commit][obv] Use TYPE_LENGTH directly where possible References: <20120926132621.5f45acb7@spoyarek> <20120926094227.GA4335@adacore.com> <20120926152740.0900e9b7@spoyarek> <20120926105205.GB4335@adacore.com> 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: 2012-09/txt/msg00664.txt.bz2 > Date: Wed, 26 Sep 2012 12:52:06 +0200 > From: Joel Brobecker > > > > Why is the type not OK for the assert, and yet OK for the rest of > > > the code? (the same question applies to other files, as well) > > > > This is so that the assert is not subject to any truncation/overflow > > resulting due to the type of LEN. That way, I don't have to expand LEN > > since I know that the value is always going to be less than 16 and if > > something actually goes wrong, then the assert will definitely catch it. > > OK. I see why it works. > > But I can definitely see someone like myself missing that subtlety, > and commit an obvious change that reduces the duplication by re-using > the variable in the gdb_assert call. To the unattentive me, that's > an obvious improvement. I think that's an issue. > > > It does not make any functional difference at all and the justification > > is in fact that it reduces the size of the bitpos patch. I have > > committed similar changes in the past that were deemed to be OK, so I > > don't see why this patch in particular should be a problem. > > I know it makes no functional difference, but it does make a noticeable > difference in terms of maintenance, IMO. And I have in fact been > silently grumbling about your patches being labeled "obvious" when > in fact I do not consider them obvious. But I have let them go, because > it wasn't significant enough that I felt I needed to talk about it. > > But this part of this patch in particular did catch my attention, and > I feel that we need to discuss it. I think this goes in the wrong > direction, and it would be better to just change the variable > (constant???) type, rather than duplicate the expression everywhere. > I don't have a strong opinion on this, and if other maintainers are ok > with it, then OK. But in the meantime, I think the previous version > was better. > > > I have not made changes in places where more than 4-5 substitutions > > were necessary and where the code started looking unwieldy as a > > result. I guess both those parameters are subjective, but I couldn't > > see a coding convention that seems to have been strictly followed > > throughout the code base. > > I agree it's subjective. Just FYI, my tolerance starts at 2. If I need > to repeat an expression, I often start thinking about factorizing into > constants, functions, etc (duplication is not the only part of the > decision process, so I don't necessarily do it). I agree with Joel. Actually my tolerance starts at 1, if it avoids having lines that are too long or if it reduces the number of nested parentheses to a more manageable level.