From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 58048 invoked by alias); 30 Oct 2017 16:05:36 -0000 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 Received: (qmail 58035 invoked by uid 89); 30 Oct 2017 16:05:36 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.0 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS,SPF_SOFTFAIL autolearn=ham version=3.3.2 spammy=stuck X-HELO: mail.baldwin.cx Received: from bigwig.baldwin.cx (HELO mail.baldwin.cx) (96.47.65.170) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 30 Oct 2017 16:05:34 +0000 Received: from dhcp-10-248-127-128.eduroam.wireless.private.cam.ac.uk (global-5-144.nat-2.net.cam.ac.uk [131.111.5.144]) by mail.baldwin.cx (Postfix) with ESMTPSA id 0662A10A8BC; Mon, 30 Oct 2017 12:05:31 -0400 (EDT) Subject: Re: [PATCH] Introduce in_inclusive_range, fix -Wtautological-compare warnings To: Simon Marchi References: <189cefb7-aa0c-16bd-d78c-cc6f1e5c1344@baldwin.cx> <1509373931-16340-1-git-send-email-simon.marchi@ericsson.com> Cc: Simon Marchi , gdb-patches@sourceware.org From: John Baldwin Message-ID: <414aebc6-2dc6-d2b2-a1b4-35f98adb78f9@FreeBSD.org> Date: Mon, 30 Oct 2017 16:05:00 -0000 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2017-10/txt/msg00903.txt.bz2 On 10/30/17 4:01 PM, Simon Marchi wrote: > On 2017-10-30 11:57, John Baldwin wrote: >> On 10/30/17 2:32 PM, Simon Marchi wrote: >>> When compiling with clang or gcc 8, we see warnings like this: >>> >>> /home/emaisin/src/binutils-gdb/gdb/arm-tdep.c:10013:13: error: >>> comparison of 0 <= unsigned expression is always true >>> [-Werror,-Wtautological-compare] >>> if (0 <= insn_op1 && 3 >= insn_op1) >>> ~ ^ ~~~~~~~~ >>> /home/emaisin/src/binutils-gdb/gdb/arm-tdep.c:11722:20: error: >>> comparison of unsigned expression >= 0 is always true >>> [-Werror,-Wtautological-compare] >>> else if (opB >= 0 && opB <= 2) >>> ~~~ ^ ~ >>> >>> This is because an unsigned integer (opB in this case) will always be >>>> = >>> 0. It is still useful to keep both bounds of the range in the >>> expression, even if one is at the edge of the data type range. This >>> patch introduces a utility function in_inclusive_range that gets rid >>> of >>> the warning while conveying that we are checking for a range. >>> >>> Tested by rebuilding. >>> >>> gdb/ChangeLog: >>> >>> * common/common-utils.h (in_inclusive_range): New function. >>> * arm-tdep.c (arm_record_extension_space): Use >>> in_inclusive_range. >>> * cris-tdep.c (cris_spec_reg_applicable): Use >>> in_inclusive_range. >>> --- >>> gdb/arm-tdep.c | 4 ++-- >>> gdb/common/common-utils.h | 9 +++++++++ >>> gdb/cris-tdep.c | 4 ++-- >>> 3 files changed, 13 insertions(+), 4 deletions(-) >>> >>> diff --git a/gdb/cris-tdep.c b/gdb/cris-tdep.c >>> index d623eb6..209e29f 100644 >>> --- a/gdb/cris-tdep.c >>> +++ b/gdb/cris-tdep.c >>> @@ -1434,7 +1434,7 @@ cris_spec_reg_applicable (struct gdbarch >>> *gdbarch, >>> /* Indeterminate/obsolete. */ >>> return 0; >>> case cris_ver_v0_3: >>> - return (version >= 0 && version <= 3); >>> + return in_inclusive_range (version, 0U, 3U); >>> case cris_ver_v3p: >>> return (version >= 3); >>> case cris_ver_v8: >>> @@ -1442,7 +1442,7 @@ cris_spec_reg_applicable (struct gdbarch >>> *gdbarch, >>> case cris_ver_v8p: >>> return (version >= 8); >>> case cris_ver_v0_10: >>> - return (version >= 0 && version <= 10); >>> + return in_inclusive_range (version, 0U, 10U); >>> case cris_ver_v3_10: >>> return (version >= 3 && version <= 10); >>> case cris_ver_v8_10: >> >> I wonder if in this file it wouldn't be best to use the new function >> throughout >> the various cases so that the style is more consistent? LGTM >> regardless. > > > Good point, I'll make that change. I'll see if it's possible in > arm-tdep.c too or if it's a too daunting task. It may be sufficient to just be consistent within a function? In arm-tdep.c it is two instances in separate functions whereas for cris-tdep.c it is multiple instances in the same function which is what stuck out to me. -- John Baldwin