From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17041 invoked by alias); 26 Sep 2012 14:59:17 -0000 Received: (qmail 17033 invoked by uid 22791); 26 Sep 2012 14:59:16 -0000 X-SWARE-Spam-Status: No, hits=-8.0 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,RP_MATCHES_RCVD,SPF_HELO_PASS X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 26 Sep 2012 14:58:59 +0000 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q8QEwvGn020537 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Wed, 26 Sep 2012 10:58:58 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id q8QEwuCB031935; Wed, 26 Sep 2012 10:58:56 -0400 Message-ID: <5063182F.3040404@redhat.com> Date: Wed, 26 Sep 2012 14:59:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20120911 Thunderbird/15.0.1 MIME-Version: 1.0 To: Joel Brobecker CC: Siddhesh Poyarekar , gdb-patches@sourceware.org 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> In-Reply-To: <20120926105205.GB4335@adacore.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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/msg00588.txt.bz2 On 09/26/2012 11:52 AM, Joel Brobecker wrote: >>> 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 agree with Joel, and was also silently glumbling. :-) > >> 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). -- Pedro Alves