From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6049 invoked by alias); 26 Sep 2012 10:52:23 -0000 Received: (qmail 6037 invoked by uid 22791); 26 Sep 2012 10:52:22 -0000 X-SWARE-Spam-Status: No, hits=-1.9 required=5.0 tests=AWL,BAYES_00,RCVD_IN_HOSTKARMA_NO 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; Wed, 26 Sep 2012 10:52:09 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 3814B1C7CA1; Wed, 26 Sep 2012 06:52:09 -0400 (EDT) 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 7-h5vs2OUcsW; Wed, 26 Sep 2012 06:52:09 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id F2AE81C7BFC; Wed, 26 Sep 2012 06:52:08 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id 0FD58C4A48; Wed, 26 Sep 2012 12:52:06 +0200 (CEST) Date: Wed, 26 Sep 2012 10:52:00 -0000 From: Joel Brobecker To: Siddhesh Poyarekar Cc: gdb-patches@sourceware.org Subject: Re: [commit][obv] Use TYPE_LENGTH directly where possible Message-ID: <20120926105205.GB4335@adacore.com> References: <20120926132621.5f45acb7@spoyarek> <20120926094227.GA4335@adacore.com> <20120926152740.0900e9b7@spoyarek> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20120926152740.0900e9b7@spoyarek> User-Agent: Mutt/1.5.21 (2010-09-15) 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/msg00577.txt.bz2 > > 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). -- Joel