From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 24067 invoked by alias); 16 May 2012 03:50:11 -0000 Received: (qmail 24047 invoked by uid 22791); 16 May 2012 03:50:10 -0000 X-SWARE-Spam-Status: No, hits=-7.4 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,T_RP_MATCHES_RCVD 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, 16 May 2012 03:49:55 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q4G3ntb9015133 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 15 May 2012 23:49:55 -0400 Received: from spoyarek (vpn-228-196.phx2.redhat.com [10.3.228.196]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id q4G3nale015441; Tue, 15 May 2012 23:49:39 -0400 Date: Wed, 16 May 2012 03:50:00 -0000 From: Siddhesh Poyarekar To: Jan Kratochvil Cc: gdb-patches@sourceware.org, Tom Tromey Subject: Re: [PATCH v2] Expand bitpos and type.length to LONGEST and ULONGEST Message-ID: <20120516092012.4acba735@spoyarek> In-Reply-To: <20120515200454.GA11338@host2.jankratochvil.net> References: <20120220132724.GB4753@spoyarek.pnq.redhat.com> <20120221210235.GA26897@host2.jankratochvil.net> <20120504183858.67d416b7@spoyarek> <20120515200454.GA11338@host2.jankratochvil.net> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII 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-05/txt/msg00594.txt.bz2 On Tue, 15 May 2012 22:04:54 +0200, Jan wrote: > while I have a local patch for it it is easier to state that: > * the patch needs a small rebase to HEAD for very recent changes. > * it does not compile on 32-bit i386 host OS due to %ld used for > LONGEST. > Thanks for the very exhaustive review. I'll need some time to go through all of the points below, but I'll come up with a new patch once I have. > In such case I think we can keep signedness for all the changes. > While one may consider more clean to use unsigned types in some cases > it can bring unexpected regressions, for example in > amd64_push_arguments you correctly changed the first occurence: > > > - int len = TYPE_LENGTH (type); > > + LONGEST len = TYPE_LENGTH (type); > > But in other cases you use ULONGEST. In this case ULONGEST would be > a regression as there is below: > > for (j = 0; len > 0; j++, len -= 8) > > If you have carefully checked all the signedness changes for possible > regressions I will re-review it. Still it seems to me unrelated to > this patch (and possibly not worth the effort, YMMV). > Or did you see some specific splint warnings for it? > I have marked in this patch such lines by ^X$. > I understand changing to patch to keep the signedness looks like > worsening the patch. I think I agree with you on this because I had not thought of the difficulty involved in isolating regressions due to the size of this patch. It would be safer in this case to keep length as LONGEST because while I did try to check for all cases where ULONGEST may cause a regression (like above), but I cannot say for sure that it's all perfect. The splint warnings are actually for the opposite (for length not being ULONGEST), since splint is not that smart in these aspects. Funnily, I think it will be easier because splint will be happy about the fact that both bitpos and length are the same type. I'll work on that and the rest of the review comments and come up with the next draft for this patch. Thanks, Siddhesh